From 4197ea545f926b0663cba927f784d14539da04ae Mon Sep 17 00:00:00 2001 From: AHNINE Amine <38895133+ahnineamine@users.noreply.github.com> Date: Sun, 7 Jun 2026 07:26:30 +0200 Subject: [PATCH] fix(hooks): stop false loop warnings and repeated identical context warnings (#2121) * fix(hooks): stop false loop warnings and repeated identical context warnings Two PostToolUse monitor defects surfaced during a long single-turn session: 1. ecc-metrics-bridge hashToolCall fingerprinted Edit/Write/MultiEdit on file_path ONLY, so several distinct edits to the same file produced the same hash and tripped the loop detector ("stuck loop") even though every edit was different. Now the hash includes the edit content (old_string/new_string/content/edits) so distinct edits to one file hash differently; identical edits still collide as intended. 2. ecc-context-monitor re-emitted the SAME warning every DEBOUNCE_CALLS (5) tool calls even when nothing changed. Because the cost figure only refreshes at Stop (turn) boundaries, a single stale value printed the identical warning ~20 times within one turn. Dedupe on message content instead: a warning surfaces only when its text changes (cost moved, new file count, new loop) or on first escalation to critical, and is otherwise suppressed. Adds regression tests for the same-file/different-content hash case. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(hooks): address CodeRabbit review (#2121) - ecc-context-monitor: clear dedupe state when warnings resolve, so the same warning text recurring in a later turn (context dips/recovers/dips, a loop that stops then restarts) is surfaced again instead of suppressed as a duplicate. Guarded so the no-warning hot path stays write-free. - ecc-metrics-bridge: hash the FULL serialized edit payload and truncate the digest, not the input. Slicing the serialized string to HASH_INPUT_LIMIT first could collapse large edits sharing their first 2048 chars, reviving the false-loop collision for big Write/edit payloads. - Add regression test for >2048-char edit divergence. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- scripts/hooks/ecc-context-monitor.js | 50 ++++++++++++++++---------- scripts/hooks/ecc-metrics-bridge.js | 21 +++++++++++ tests/hooks/ecc-metrics-bridge.test.js | 33 +++++++++++++++++ 3 files changed, 85 insertions(+), 19 deletions(-) diff --git a/scripts/hooks/ecc-context-monitor.js b/scripts/hooks/ecc-context-monitor.js index 9cdedbe3..a2475517 100644 --- a/scripts/hooks/ecc-context-monitor.js +++ b/scripts/hooks/ecc-context-monitor.js @@ -23,7 +23,6 @@ const COST_CRITICAL_USD = 50; const FILES_WARNING_COUNT = 20; const LOOP_THRESHOLD = 3; const STALE_SECONDS = 60; -const DEBOUNCE_CALLS = 5; function isEnabledEnv(value, defaultValue = true) { if (value === undefined || value === null || String(value).trim() === '') { @@ -57,7 +56,7 @@ function readWarnState(sessionId) { try { return JSON.parse(fs.readFileSync(getWarnPath(sessionId), 'utf8')); } catch { - return { callsSinceWarn: 0, lastSeverity: null }; + return { callsSinceWarn: 0, lastSeverity: null, lastMessage: null }; } } @@ -218,32 +217,45 @@ function run(rawInput) { const evalBridge = isStale ? { ...bridge, context_remaining_pct: null } : bridge; const warnings = evaluateConditions(evalBridge, { costWarnings: costWarningsEnabled() }); - if (warnings.length === 0) return rawInput; - - // Debounce logic - const warnState = readWarnState(sessionId); - warnState.callsSinceWarn = (warnState.callsSinceWarn || 0) + 1; - - const topSeverity = severityLabel(warnings[0].severity); - const severityEscalated = topSeverity === 'critical' && warnState.lastSeverity !== 'critical'; - - const isFirst = !warnState.lastSeverity; - if (!isFirst && warnState.callsSinceWarn < DEBOUNCE_CALLS && !severityEscalated) { - writeWarnState(sessionId, warnState); + if (warnings.length === 0) { + // Clear dedupe state when the condition resolves, so the SAME warning text + // recurring later (context dips, recovers, dips again; a loop that stops + // then restarts) is surfaced again instead of being suppressed as a + // duplicate. Only write when there is state to clear — most tool calls + // have no warning, and this keeps the common path free of disk writes. + const prior = readWarnState(sessionId); + if (prior.lastMessage) { + writeWarnState(sessionId, { callsSinceWarn: 0, lastSeverity: null, lastMessage: null }); + } return rawInput; } - // Reset debounce, emit warning - warnState.callsSinceWarn = 0; - warnState.lastSeverity = topSeverity; - writeWarnState(sessionId, warnState); - // Combine top 2 warnings const message = warnings .slice(0, 2) .map(w => w.message) .join('\n'); + // Dedupe on message content, not a call counter. The previous logic + // re-emitted the *same* warning every DEBOUNCE_CALLS tool calls, so a + // single unchanged condition (e.g. a cost figure that only refreshes at + // turn boundaries) printed the identical line ~20 times in one turn. Now a + // warning is surfaced only when its text changes (cost moved, a new file + // count, a new loop) or when we newly escalate to critical — genuinely new + // information — and is otherwise suppressed. + const warnState = readWarnState(sessionId); + const topSeverity = severityLabel(warnings[0].severity); + const escalatedToCritical = topSeverity === 'critical' && warnState.lastSeverity !== 'critical'; + const sameMessage = warnState.lastMessage === message; + + if (sameMessage && !escalatedToCritical) { + return rawInput; + } + + warnState.lastSeverity = topSeverity; + warnState.lastMessage = message; + writeWarnState(sessionId, warnState); + const output = { hookSpecificOutput: { hookEventName: 'PostToolUse', diff --git a/scripts/hooks/ecc-metrics-bridge.js b/scripts/hooks/ecc-metrics-bridge.js index f509cade..bd8cb39d 100644 --- a/scripts/hooks/ecc-metrics-bridge.js +++ b/scripts/hooks/ecc-metrics-bridge.js @@ -48,6 +48,27 @@ function hashToolCall(toolName, toolInput) { let key = ''; if (name === 'Bash') { key = String(toolInput?.command || '').slice(0, 160); + } else if (/^(Edit|MultiEdit|Write|NotebookEdit)$/.test(name)) { + // Fingerprint the actual change, not just the path. Hashing on file_path + // alone made every distinct edit to the same file collide, so a few normal + // edits to one file looked like a stuck loop. Include the edit content so + // different edits to the same file hash differently. + // Hash the FULL serialized payload (truncate the digest, not the input): + // slicing the serialized string to HASH_INPUT_LIMIT first would collapse + // large edits that share their first N chars, reviving the same false-loop + // collision for big Write/edit payloads. + key = crypto + .createHash('sha256') + .update( + stableStringify({ + file_path: toolInput?.file_path, + old_string: toolInput?.old_string, + new_string: toolInput?.new_string, + content: toolInput?.content, + edits: toolInput?.edits + }) + ) + .digest('hex'); } else if (toolInput?.file_path) { key = String(toolInput.file_path); } else { diff --git a/tests/hooks/ecc-metrics-bridge.test.js b/tests/hooks/ecc-metrics-bridge.test.js index cdd99535..3046f040 100644 --- a/tests/hooks/ecc-metrics-bridge.test.js +++ b/tests/hooks/ecc-metrics-bridge.test.js @@ -78,6 +78,39 @@ function runTests() { passed++; else failed++; + if ( + test('different edits to the SAME file produce different hashes (no false loop)', () => { + const h1 = hashToolCall('Edit', { file_path: 'a.kt', old_string: 'foo', new_string: 'bar' }); + const h2 = hashToolCall('Edit', { file_path: 'a.kt', old_string: 'baz', new_string: 'qux' }); + assert.notStrictEqual(h1, h2); + }) + ) + passed++; + else failed++; + + if ( + test('identical Edit (same file + same change) still hashes the same', () => { + const args = { file_path: 'a.kt', old_string: 'foo', new_string: 'bar' }; + assert.strictEqual(hashToolCall('Edit', args), hashToolCall('Edit', { ...args })); + }) + ) + passed++; + else failed++; + + if ( + test('large edits diverging only after 2048 chars still hash differently', () => { + // Shared prefix longer than the old HASH_INPUT_LIMIT (2048) truncation + // point; the payloads differ only afterwards. Hashing the full payload + // (digest truncated, not input) must keep them distinct. + const prefix = 'x'.repeat(4000); + const h1 = hashToolCall('Write', { file_path: 'big.txt', content: prefix + 'AAA' }); + const h2 = hashToolCall('Write', { file_path: 'big.txt', content: prefix + 'BBB' }); + assert.notStrictEqual(h1, h2); + }) + ) + passed++; + else failed++; + if ( test('non-file tools hash by stable input to avoid false loop collisions', () => { const h1 = hashToolCall('Glob', { pattern: '**/*.js', path: '/repo/a' });