From 086e44c9641f288437f4601278a42917bda16407 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Mon, 18 May 2026 10:03:21 +0900 Subject: [PATCH] fix(hooks): log fail-open breadcrumb on parse/read errors in metrics bridge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit coderabbitai flagged: the two `catch` blocks in `readSessionCost` silently swallowed every failure mode. A malformed `costs.jsonl` row, a permission error opening the file, or any other unexpected I/O failure would silently return zero cost — masking real problems and feeding stale or zero numbers into `ecc-context-monitor.js` (which then injects them as `additionalContext` into the live model turn). Fix two things, both fail-open-preserving: 1. **Inner JSON.parse catch** — count malformed lines and write one aggregated breadcrumb per call: [ecc-metrics-bridge] skipped N malformed line(s) in Aggregating (rather than per-line) keeps a log-flooded `costs.jsonl` diagnosable without overwhelming stderr. 2. **Outer fs.readFileSync catch** — write a breadcrumb on real errors, but stay silent on `ENOENT`. The "no costs.jsonl yet" case is genuinely normal (no Stop event has fired this session) and producing noise on every PreToolUse before the first Stop would be reviewer-visible spam. All other error codes (`EACCES`, `EISDIR`, `EMFILE`, …) get: [ecc-metrics-bridge] failing open after reading : In both cases the function still returns the zero-cost fallback so the bridge never breaks tool execution — only the diagnosability changes. Two new regression tests in `tests/hooks/ecc-metrics-bridge.test.js`: ✓ readSessionCost writes a stderr breadcrumb when malformed lines are skipped — feeds 4 rows (2 valid, 2 malformed), asserts the last valid row still wins AND captured stderr contains "skipped 2 malformed line(s)". ✓ readSessionCost stays silent when costs.jsonl does not exist (ENOENT) — uses a fresh tmp HOME with no metrics dir, asserts zero return AND empty stderr. Test count: 16 → 18; `npm test` green; `yarn lint` clean. --- scripts/hooks/ecc-metrics-bridge.js | 19 +++++- tests/hooks/ecc-metrics-bridge.test.js | 83 ++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 3 deletions(-) diff --git a/scripts/hooks/ecc-metrics-bridge.js b/scripts/hooks/ecc-metrics-bridge.js index 5b2c05e6..241824f9 100644 --- a/scripts/hooks/ecc-metrics-bridge.js +++ b/scripts/hooks/ecc-metrics-bridge.js @@ -94,14 +94,15 @@ function extractFilePaths(toolName, toolInput) { * even cheaper. */ function readSessionCost(sessionId) { + const costsPath = path.join(getClaudeDir(), 'metrics', 'costs.jsonl'); try { - const costsPath = path.join(getClaudeDir(), 'metrics', 'costs.jsonl'); const content = fs.readFileSync(costsPath, 'utf8'); const lines = content.split('\n').filter(Boolean); let totalCost = 0; let totalIn = 0; let totalOut = 0; + let malformed = 0; for (const line of lines) { try { const row = JSON.parse(line); @@ -111,11 +112,23 @@ function readSessionCost(sessionId) { totalOut = toNumber(row.output_tokens); } } catch { - /* skip malformed lines */ + malformed += 1; } } + // One aggregated breadcrumb per call rather than one per bad row, so a + // log-flooded costs.jsonl stays diagnosable without overwhelming stderr. + if (malformed > 0) { + process.stderr.write(`[ecc-metrics-bridge] skipped ${malformed} malformed line(s) in ${costsPath}\n`); + } return { totalCost, totalIn, totalOut }; - } catch { + } catch (err) { + // ENOENT is the common case (no Stop event has fired yet this session) + // and is not actually a failure — stay silent on it. Anything else + // (permission, EISDIR, malformed read) deserves a breadcrumb because + // the bridge will silently report zero cost otherwise. + if (err && err.code !== 'ENOENT') { + process.stderr.write(`[ecc-metrics-bridge] failing open after ${err.name || 'error'} reading ${costsPath}: ${err.message || String(err)}\n`); + } return { totalCost: 0, totalIn: 0, totalOut: 0 }; } } diff --git a/tests/hooks/ecc-metrics-bridge.test.js b/tests/hooks/ecc-metrics-bridge.test.js index 109e0b6c..241022ff 100644 --- a/tests/hooks/ecc-metrics-bridge.test.js +++ b/tests/hooks/ecc-metrics-bridge.test.js @@ -225,6 +225,89 @@ function runTests() { passed++; else failed++; + if ( + test('readSessionCost writes a stderr breadcrumb when malformed lines are skipped', () => { + // Reviewer (coderabbitai) asked for diagnosability when the inner + // catch silently skips malformed JSON rows. Verify the aggregated + // "skipped N malformed line(s)" breadcrumb appears on stderr while + // the function still recovers the last valid matching row. + const tmpHome = makeTempHome(); + const originalHome = process.env.HOME; + const originalUserProfile = process.env.USERPROFILE; + const originalStderrWrite = process.stderr.write.bind(process.stderr); + let captured = ''; + process.stderr.write = chunk => { + captured += String(chunk); + return true; + }; + try { + process.env.HOME = tmpHome; + process.env.USERPROFILE = tmpHome; + const metricsDir = path.join(tmpHome, '.claude', 'metrics'); + fs.mkdirSync(metricsDir, { recursive: true }); + fs.writeFileSync( + path.join(metricsDir, 'costs.jsonl'), + [ + JSON.stringify({ session_id: 'S1', estimated_cost_usd: 0.5, input_tokens: 500, output_tokens: 250 }), + 'NOT_JSON', + '{"truncated":', + JSON.stringify({ session_id: 'S1', estimated_cost_usd: 0.7, input_tokens: 700, output_tokens: 350 }), + ].join('\n') + '\n', + 'utf8' + ); + const result = readSessionCost('S1'); + assert.strictEqual(result.totalCost, 0.7, 'last valid row should still win'); + assert.ok(/skipped 2 malformed line\(s\)/.test(captured), + `expected aggregated malformed-line breadcrumb on stderr, got: ${captured}`); + } finally { + process.stderr.write = originalStderrWrite; + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = originalUserProfile; + fs.rmSync(tmpHome, { recursive: true, force: true }); + } + }) + ) + passed++; + else failed++; + + if ( + test('readSessionCost stays silent when costs.jsonl does not exist (ENOENT)', () => { + // ENOENT is the common case before any Stop event has fired — it is + // not a failure and should not produce stderr noise. Other errors + // (permission, EISDIR, etc.) DO produce a breadcrumb, covered by the + // malformed-line test above's surrounding harness. + const tmpHome = makeTempHome(); + const originalHome = process.env.HOME; + const originalUserProfile = process.env.USERPROFILE; + const originalStderrWrite = process.stderr.write.bind(process.stderr); + let captured = ''; + process.stderr.write = chunk => { + captured += String(chunk); + return true; + }; + try { + process.env.HOME = tmpHome; + process.env.USERPROFILE = tmpHome; + // Do NOT create the metrics dir or file — readSessionCost should + // hit ENOENT and return zeros silently. + const result = readSessionCost('S1'); + assert.deepStrictEqual(result, { totalCost: 0, totalIn: 0, totalOut: 0 }); + assert.strictEqual(captured, '', `expected no stderr on ENOENT, got: ${captured}`); + } finally { + process.stderr.write = originalStderrWrite; + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = originalUserProfile; + fs.rmSync(tmpHome, { recursive: true, force: true }); + } + }) + ) + passed++; + else failed++; + if ( test('readSessionCost does not include unrelated default-session rows', () => { const tmpHome = makeTempHome();