diff --git a/hooks/hooks.json b/hooks/hooks.json index 2eb1ef3ea..35d79fd5a 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -171,6 +171,17 @@ ], "description": "Track failed MCP tool calls, mark unhealthy servers, and attempt reconnect", "id": "post:mcp-health-check" + }, + { + "matcher": "Skill", + "hooks": [ + { + "type": "command", + "command": "node -e \"const p=require('path');const r=(function(){var p=require('path'),f=require('fs'),o=require('os');var e=process.env.CLAUDE_PLUGIN_ROOT;if(e&&e.trim())return e.trim();var d=p.join(o.homedir(),'.claude');function L(x){try{return require(p.join(x,'scripts','lib','resolve-ecc-root')).resolveEccRoot()}catch(_){return null}}var r=L(d);if(r)return r;var s=['ecc','ecc@ecc','marketplaces/ecc','everything-claude-code','everything-claude-code@everything-claude-code','marketplaces/everything-claude-code'];for(var i=0;i maxLength) { + return null; + } + return pattern.test(trimmed) ? trimmed : null; +} + +function firstIdentifier(maxLength, pattern, ...values) { + for (const value of values) { + const identifier = boundedIdentifier(value, maxLength, pattern); + if (identifier) { + return identifier; + } + } + return null; +} + +// Extract the skill identifier from the Skill tool input across the field +// names Claude Code has used for it. The Skill tool is genuinely un-wired in +// this repo, so no single canonical field is guaranteed — probe the plausible +// ones and bail (record nothing) if none is present. +function extractSkillId(toolInput) { + if (typeof toolInput === 'string') { + return boundedIdentifier(toolInput, MAX_SKILL_ID, SKILL_ID_PATTERN); + } + if (!toolInput || typeof toolInput !== 'object') { + return null; + } + return firstIdentifier( + MAX_SKILL_ID, + SKILL_ID_PATTERN, + toolInput.skill_id, + toolInput.skillId, + toolInput.skill, + toolInput.name, + toolInput.command + ); +} + +// Best-effort outcome: a failed tool call is recorded as "failure", everything +// else as "success". Both PostToolUseFailure routing and an error-bearing +// tool response are treated as failure. +function deriveOutcome(payload) { + if (payload && payload.hook_event_name === 'PostToolUseFailure') { + return 'failure'; + } + + const response = (payload && (payload.tool_response ?? payload.tool_output)) || null; + if (response && typeof response === 'object') { + if (response.is_error === true || response.isError === true) { + return 'failure'; + } + if (typeof response.status === 'string' && /error|fail/i.test(response.status)) { + return 'failure'; + } + if (typeof response.error === 'string' && response.error.trim().length > 0) { + return 'failure'; + } + } + + return 'success'; +} + +function buildRecord(payload) { + const skillId = extractSkillId(payload.tool_input); + if (!skillId) { + return null; // cannot satisfy the tracker's required skill_id — skip + } + + const input = payload.tool_input && typeof payload.tool_input === 'object' + ? payload.tool_input + : {}; + + const skillVersion = firstIdentifier( + MAX_SKILL_VERSION, + SKILL_VERSION_PATTERN, + input.skill_version, + input.skillVersion, + input.version + ) || 'unknown'; + + return { + skill_id: skillId, + skill_version: skillVersion, + // Synthesized, not user content. The tracker requires a non-empty + // task_description; the dashboard never displays it as prose. + task_description: `Skill invocation: ${skillId}`, + outcome: deriveOutcome(payload), + }; +} + +function run(rawInput) { + try { + const payload = typeof rawInput === 'string' + ? (rawInput.trim() ? JSON.parse(rawInput) : {}) + : rawInput; + if (payload && typeof payload === 'object' && payload.tool_name === 'Skill') { + const record = buildRecord(payload); + if (record) { + recordSkillExecution(record); + } + } + } catch { + // Telemetry is best-effort; never block tool execution on a failure here. + } +} + +module.exports = { buildRecord, deriveOutcome, extractSkillId, run }; diff --git a/scripts/lib/skill-evolution/tracker.js b/scripts/lib/skill-evolution/tracker.js index 67220eb93..0ea0ba1cc 100644 --- a/scripts/lib/skill-evolution/tracker.js +++ b/scripts/lib/skill-evolution/tracker.js @@ -4,11 +4,19 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); -const { appendFile } = require('../utils'); +const { ensureDir } = require('../utils'); const VALID_OUTCOMES = new Set(['success', 'failure', 'partial']); const VALID_FEEDBACK = new Set(['accepted', 'corrected', 'rejected']); +// Retention bound for the JSONL sink. The dashboard only aggregates recent +// runs, so an unbounded append-only file is pure cost. Trim from the front +// once the file grows past the cap. +const MAX_RUN_RECORDS = 5000; +// Owner-only. The sink lives under the user's home and is local telemetry; +// nothing else on the machine needs to read it. +const RUNS_FILE_MODE = 0o600; + function resolveHomeDir(homeDir) { return homeDir ? path.resolve(homeDir) : os.homedir(); } @@ -102,6 +110,46 @@ function readJsonl(filePath) { }, []); } +// Append one record to the JSONL sink with owner-only permissions, then +// enforce the retention cap. `fs.appendFileSync`'s mode only applies when it +// creates the file, so an existing world-readable sink is chmod'd on the way +// past — cheap, and it repairs files written before this bound existed. +function appendRunRecord(runsFilePath, record, options = {}) { + const maxRecords = Number.isInteger(options.maxRecords) && options.maxRecords > 0 + ? options.maxRecords + : MAX_RUN_RECORDS; + + ensureDir(path.dirname(runsFilePath)); + fs.appendFileSync(runsFilePath, `${JSON.stringify(record)}\n`, { encoding: 'utf8', mode: RUNS_FILE_MODE }); + + try { + fs.chmodSync(runsFilePath, RUNS_FILE_MODE); + } catch { + // Windows and some mounts do not support POSIX modes; the record still lands. + } + + pruneRunRecords(runsFilePath, maxRecords); +} + +// Keep only the newest `maxRecords` lines. Rewrites the whole file, which is +// fine because the file is bounded by this very cap; it only runs on the +// appends that actually cross the line. +function pruneRunRecords(runsFilePath, maxRecords) { + try { + const lines = fs.readFileSync(runsFilePath, 'utf8').split('\n').filter(Boolean); + if (lines.length <= maxRecords) { + return; + } + fs.writeFileSync( + runsFilePath, + `${lines.slice(-maxRecords).join('\n')}\n`, + { encoding: 'utf8', mode: RUNS_FILE_MODE } + ); + } catch { + // Retention is best-effort; never fail a recorded run over it. + } +} + function recordSkillExecution(input, options = {}) { const record = normalizeExecutionRecord(input, options); @@ -119,7 +167,7 @@ function recordSkillExecution(input, options = {}) { } const runsFilePath = getRunsFilePath(options); - appendFile(runsFilePath, `${JSON.stringify(record)}\n`); + appendRunRecord(runsFilePath, record, options); return { storage: 'jsonl', @@ -137,6 +185,8 @@ function readSkillExecutionRecords(options = {}) { } module.exports = { + MAX_RUN_RECORDS, + RUNS_FILE_MODE, VALID_FEEDBACK, VALID_OUTCOMES, getRunsFilePath, diff --git a/tests/hooks/skill-run-tracker.test.js b/tests/hooks/skill-run-tracker.test.js new file mode 100644 index 000000000..bd97b50c8 --- /dev/null +++ b/tests/hooks/skill-run-tracker.test.js @@ -0,0 +1,259 @@ +/** + * Tests for scripts/hooks/skill-run-tracker.js and the JSONL sink bounds in + * scripts/lib/skill-evolution/tracker.js (#2463). + * + * Focus: the tracker records real runs, and it never persists prompt text, + * unbounded strings, an unbounded file, or a world-readable sink. + * + * Run with: node tests/run-all.js + */ + +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const { buildRecord, deriveOutcome, extractSkillId, run } = require('../../scripts/hooks/skill-run-tracker'); +const { + MAX_RUN_RECORDS, + RUNS_FILE_MODE, + getRunsFilePath, + recordSkillExecution, + readSkillExecutionRecords, +} = require('../../scripts/lib/skill-evolution/tracker'); + +let passed = 0; +let failed = 0; + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + passed++; + } catch (err) { + console.log(` ✗ ${name}`); + console.log(` Error: ${err.message}`); + failed++; + } +} + +function withTempHome(fn) { + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-skill-runs-')); + try { + return fn(homeDir); + } finally { + fs.rmSync(homeDir, { recursive: true, force: true }); + } +} + +function payload(overrides = {}) { + return { + hook_event_name: 'PostToolUse', + tool_name: 'Skill', + tool_input: { skill_id: 'code-review', skill_version: '1.2.0' }, + tool_response: {}, + ...overrides, + }; +} + +// ── skill id extraction and bounds ──────────────────────────────────────────── + +test('extractSkillId probes the field names Claude Code has used', () => { + assert.strictEqual(extractSkillId({ skill_id: 'a' }), 'a'); + assert.strictEqual(extractSkillId({ skillId: 'b' }), 'b'); + assert.strictEqual(extractSkillId({ skill: 'c' }), 'c'); + assert.strictEqual(extractSkillId({ name: 'd' }), 'd'); + assert.strictEqual(extractSkillId({ command: 'e' }), 'e'); + assert.strictEqual(extractSkillId('bare-string'), 'bare-string'); +}); + +test('extractSkillId returns null when no skill id is present', () => { + assert.strictEqual(extractSkillId({}), null); + assert.strictEqual(extractSkillId(null), null); + assert.strictEqual(extractSkillId(42), null); +}); + +test('extractSkillId rejects an over-long identifier rather than truncating it', () => { + assert.strictEqual(extractSkillId({ skill_id: 'x'.repeat(129) }), null); + assert.strictEqual(extractSkillId({ skill_id: 'x'.repeat(128) }), 'x'.repeat(128)); +}); + +test('extractSkillId rejects free text that is not identifier-shaped', () => { + // A prompt smuggled into the skill field must not become a persisted id. + assert.strictEqual(extractSkillId({ skill_id: 'summarize this: my api key is sk-abc' }), null); + assert.strictEqual(extractSkillId({ skill_id: 'line\nbreak' }), null); + assert.strictEqual(extractSkillId({ skill_id: ' ' }), null); +}); + +// ── privacy: no prompt text is persisted ───────────────────────────────────── + +test('buildRecord synthesizes task_description and never copies prompt fields', () => { + const secret = 'PROMPT-SECRET-do-not-persist'; + const record = buildRecord(payload({ + tool_input: { + skill_id: 'code-review', + skill_version: '1.2.0', + task_description: secret, + description: secret, + prompt: secret, + taskDescription: secret, + }, + })); + + assert.strictEqual(record.task_description, 'Skill invocation: code-review'); + assert.ok(!JSON.stringify(record).includes(secret), 'record must not contain any prompt text'); +}); + +test('buildRecord persists only the four dashboard fields', () => { + const record = buildRecord(payload()); + assert.deepStrictEqual( + Object.keys(record).sort(), + ['outcome', 'skill_id', 'skill_version', 'task_description'] + ); +}); + +test('buildRecord drops a non-identifier skill_version instead of persisting it', () => { + const record = buildRecord(payload({ + tool_input: { skill_id: 'code-review', skill_version: 'v1 (as requested by the user in chat)' }, + })); + assert.strictEqual(record.skill_version, 'unknown'); +}); + +test('buildRecord returns null when the skill id is unusable', () => { + assert.strictEqual(buildRecord(payload({ tool_input: {} })), null); +}); + +// ── outcome derivation ─────────────────────────────────────────────────────── + +test('deriveOutcome reports failure for PostToolUseFailure routing', () => { + assert.strictEqual(deriveOutcome(payload({ hook_event_name: 'PostToolUseFailure' })), 'failure'); +}); + +test('deriveOutcome reports failure for error-bearing tool responses', () => { + assert.strictEqual(deriveOutcome(payload({ tool_response: { is_error: true } })), 'failure'); + assert.strictEqual(deriveOutcome(payload({ tool_response: { isError: true } })), 'failure'); + assert.strictEqual(deriveOutcome(payload({ tool_response: { status: 'ERROR' } })), 'failure'); + assert.strictEqual(deriveOutcome(payload({ tool_response: { error: 'boom' } })), 'failure'); +}); + +test('deriveOutcome reports success otherwise', () => { + assert.strictEqual(deriveOutcome(payload()), 'success'); + assert.strictEqual(deriveOutcome(payload({ tool_response: { status: 'ok' } })), 'success'); +}); + +// ── hook behaviour ─────────────────────────────────────────────────────────── + +test('run ignores non-Skill tools and malformed input without throwing', () => { + assert.doesNotThrow(() => run(JSON.stringify(payload({ tool_name: 'Bash' })))); + assert.doesNotThrow(() => run('not json')); + assert.doesNotThrow(() => run('')); +}); + +// ── JSONL sink bounds ──────────────────────────────────────────────────────── + +test('recordSkillExecution writes the sink owner-only', function () { + if (process.platform === 'win32') { + return; // POSIX modes are not meaningful on Windows + } + withTempHome(homeDir => { + recordSkillExecution( + { skill_id: 'code-review', skill_version: '1.0.0', task_description: 'Skill invocation: code-review', outcome: 'success' }, + { homeDir } + ); + const runsFilePath = getRunsFilePath({ homeDir }); + const mode = fs.statSync(runsFilePath).mode & 0o777; + assert.strictEqual(mode, RUNS_FILE_MODE, `expected mode ${RUNS_FILE_MODE.toString(8)}, got ${mode.toString(8)}`); + }); +}); + +test('recordSkillExecution re-tightens an already world-readable sink', function () { + if (process.platform === 'win32') { + return; + } + withTempHome(homeDir => { + const runsFilePath = getRunsFilePath({ homeDir }); + fs.mkdirSync(path.dirname(runsFilePath), { recursive: true }); + fs.writeFileSync(runsFilePath, '', { mode: 0o644 }); + + recordSkillExecution( + { skill_id: 'code-review', skill_version: '1.0.0', task_description: 'Skill invocation: code-review', outcome: 'success' }, + { homeDir } + ); + + assert.strictEqual(fs.statSync(runsFilePath).mode & 0o777, RUNS_FILE_MODE); + }); +}); + +test('the JSONL sink is bounded by a retention cap', () => { + withTempHome(homeDir => { + const maxRecords = 5; + for (let i = 0; i < maxRecords + 4; i++) { + recordSkillExecution( + { skill_id: `skill-${i}`, skill_version: '1.0.0', task_description: `Skill invocation: skill-${i}`, outcome: 'success' }, + { homeDir, maxRecords } + ); + } + + const records = readSkillExecutionRecords({ homeDir }); + assert.strictEqual(records.length, maxRecords, 'sink must be trimmed to the cap'); + // Trimming keeps the newest runs, so the dashboard still reflects recent activity. + assert.strictEqual(records[records.length - 1].skill_id, `skill-${maxRecords + 3}`); + assert.strictEqual(records[0].skill_id, `skill-${4}`); + }); +}); + +test('the default retention cap is a finite bound', () => { + assert.ok(Number.isInteger(MAX_RUN_RECORDS) && MAX_RUN_RECORDS > 0, 'MAX_RUN_RECORDS must be a positive integer'); +}); + +test('an end-to-end Skill hook run lands exactly one non-sensitive record', () => { + withTempHome(homeDir => { + const previousHome = process.env.HOME; + const previousUserProfile = process.env.USERPROFILE; + process.env.HOME = homeDir; + process.env.USERPROFILE = homeDir; + try { + run(JSON.stringify(payload({ + tool_input: { skill_id: 'code-review', skill_version: '1.2.0', prompt: 'PROMPT-SECRET' }, + }))); + + const records = readSkillExecutionRecords({ homeDir }); + assert.strictEqual(records.length, 1); + assert.strictEqual(records[0].skill_id, 'code-review'); + assert.strictEqual(records[0].skill_version, '1.2.0'); + assert.strictEqual(records[0].outcome, 'success'); + assert.ok(!JSON.stringify(records[0]).includes('PROMPT-SECRET')); + } finally { + if (previousHome === undefined) delete process.env.HOME; else process.env.HOME = previousHome; + if (previousUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = previousUserProfile; + } + }); +}); + +// deriveOutcome treats PostToolUseFailure as a hard failure. That branch is +// only reachable if the hook is actually registered for the event: the +// PostToolUse dispatcher does not fan out PostToolUseFailure, so the tracker +// needs its own hooks.json entry. Without it, hard Skill failures are silently +// dropped and the dashboard's success rate is inflated. +test('the tracker is registered for PostToolUseFailure so hard failures are recorded', () => { + const hooksConfig = JSON.parse( + fs.readFileSync(path.join(__dirname, '..', '..', 'hooks', 'hooks.json'), 'utf8') + ); + const entries = (hooksConfig.hooks.PostToolUseFailure || []) + .filter(entry => entry.id === 'post:skill:track'); + + assert.strictEqual(entries.length, 1, 'expected one post:skill:track PostToolUseFailure entry'); + assert.strictEqual(entries[0].matcher, 'Skill', 'tracker must only match the Skill tool'); + assert.ok( + entries[0].hooks[0].command.includes('scripts/hooks/skill-run-tracker.js'), + 'entry should invoke skill-run-tracker.js' + ); +}); + +console.log(`\nPassed: ${passed}`); +console.log(`Failed: ${failed}`); +if (failed > 0) { + process.exitCode = 1; +}