mirror of
https://github.com/affaan-m/everything-claude-code.git
synced 2026-05-20 07:43:07 +08:00
fix(hooks): log fail-open breadcrumb on parse/read errors in metrics bridge
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 <path>
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 <name> reading <path>: <msg>
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.
This commit is contained in:
@@ -94,14 +94,15 @@ function extractFilePaths(toolName, toolInput) {
|
|||||||
* even cheaper.
|
* even cheaper.
|
||||||
*/
|
*/
|
||||||
function readSessionCost(sessionId) {
|
function readSessionCost(sessionId) {
|
||||||
|
const costsPath = path.join(getClaudeDir(), 'metrics', 'costs.jsonl');
|
||||||
try {
|
try {
|
||||||
const costsPath = path.join(getClaudeDir(), 'metrics', 'costs.jsonl');
|
|
||||||
const content = fs.readFileSync(costsPath, 'utf8');
|
const content = fs.readFileSync(costsPath, 'utf8');
|
||||||
const lines = content.split('\n').filter(Boolean);
|
const lines = content.split('\n').filter(Boolean);
|
||||||
|
|
||||||
let totalCost = 0;
|
let totalCost = 0;
|
||||||
let totalIn = 0;
|
let totalIn = 0;
|
||||||
let totalOut = 0;
|
let totalOut = 0;
|
||||||
|
let malformed = 0;
|
||||||
for (const line of lines) {
|
for (const line of lines) {
|
||||||
try {
|
try {
|
||||||
const row = JSON.parse(line);
|
const row = JSON.parse(line);
|
||||||
@@ -111,11 +112,23 @@ function readSessionCost(sessionId) {
|
|||||||
totalOut = toNumber(row.output_tokens);
|
totalOut = toNumber(row.output_tokens);
|
||||||
}
|
}
|
||||||
} catch {
|
} 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 };
|
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 };
|
return { totalCost: 0, totalIn: 0, totalOut: 0 };
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -225,6 +225,89 @@ function runTests() {
|
|||||||
passed++;
|
passed++;
|
||||||
else failed++;
|
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 (
|
if (
|
||||||
test('readSessionCost does not include unrelated default-session rows', () => {
|
test('readSessionCost does not include unrelated default-session rows', () => {
|
||||||
const tmpHome = makeTempHome();
|
const tmpHome = makeTempHome();
|
||||||
|
|||||||
Reference in New Issue
Block a user