mirror of
https://github.com/affaan-m/ECC.git
synced 2026-10-01 22:15:17 +02:00
fix: harden Hookify review follow-ups
This commit is contained in:
@@ -200,6 +200,7 @@
|
||||
],
|
||||
"Stop": [
|
||||
{
|
||||
"matcher": "*",
|
||||
"hooks": [
|
||||
{
|
||||
"type": "command",
|
||||
|
||||
@@ -50,6 +50,7 @@ const EVENT_FIELDS = Object.freeze({
|
||||
stop: new Set(['content']),
|
||||
all: new Set(['command', 'file_path', 'new_text', 'old_text', 'content', 'user_prompt']),
|
||||
});
|
||||
const MODEL_FACING_FORMAT_CONTROL = /[\u200B-\u200F\u2028\u2029\u202A-\u202E\u2066-\u2069\uFEFF]/u;
|
||||
|
||||
function diagnostic(code, fileName, detail) {
|
||||
const label = fileName ? ` ${fileName}` : '';
|
||||
@@ -73,6 +74,10 @@ function hasUnsafeControlCharacters(value, allowNewlines = false) {
|
||||
return false;
|
||||
}
|
||||
|
||||
function hasUnsafeModelFacingCharacters(value) {
|
||||
return MODEL_FACING_FORMAT_CONTROL.test(value);
|
||||
}
|
||||
|
||||
function parseQuotedScalar(rawValue) {
|
||||
if (rawValue.startsWith('"')) {
|
||||
if (!rawValue.endsWith('"')) {
|
||||
@@ -118,6 +123,7 @@ function parseFrontmatter(frontmatterText) {
|
||||
const lines = frontmatterText.split('\n');
|
||||
let conditions = null;
|
||||
let currentCondition = null;
|
||||
let insideConditions = false;
|
||||
|
||||
for (const originalLine of lines) {
|
||||
const line = originalLine.endsWith('\r')
|
||||
@@ -136,15 +142,17 @@ function parseFrontmatter(frontmatterText) {
|
||||
conditions = [];
|
||||
result.conditions = conditions;
|
||||
currentCondition = null;
|
||||
insideConditions = true;
|
||||
} else {
|
||||
setUnique(result, key, parseScalar(rawValue));
|
||||
currentCondition = null;
|
||||
insideConditions = false;
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
const listStart = line.match(/^ {2}- ([a-z_]+):(?:[ \t]*(.*))?$/);
|
||||
if (listStart && conditions) {
|
||||
if (listStart && insideConditions && conditions) {
|
||||
const [, key, rawValue = ''] = listStart;
|
||||
if (!CONDITION_FIELDS.has(key)) throw new Error(`unknown condition field ${key}`);
|
||||
currentCondition = {};
|
||||
@@ -154,7 +162,7 @@ function parseFrontmatter(frontmatterText) {
|
||||
}
|
||||
|
||||
const continuation = line.match(/^ {4}([a-z_]+):(?:[ \t]*(.*))?$/);
|
||||
if (continuation && currentCondition) {
|
||||
if (continuation && insideConditions && currentCondition) {
|
||||
const [, key, rawValue = ''] = continuation;
|
||||
if (!CONDITION_FIELDS.has(key)) throw new Error(`unknown condition field ${key}`);
|
||||
setUnique(currentCondition, key, parseScalar(rawValue));
|
||||
@@ -187,6 +195,9 @@ function validateString(value, field, options = {}) {
|
||||
if (hasUnsafeControlCharacters(value, options.allowNewlines === true)) {
|
||||
throw new Error(`${field} contains control characters`);
|
||||
}
|
||||
if (options.modelFacing === true && hasUnsafeModelFacingCharacters(value)) {
|
||||
throw new Error(`${field} contains unsafe invisible or bidirectional characters`);
|
||||
}
|
||||
if (options.maxLength && value.length > options.maxLength) {
|
||||
throw new Error(`${field} exceeds its length limit`);
|
||||
}
|
||||
@@ -287,7 +298,10 @@ function validateRule(frontmatter, message, source) {
|
||||
conditions = frontmatter.conditions.map(condition => validateCondition(condition, event));
|
||||
}
|
||||
|
||||
validateString(message, 'message', { allowNewlines: true });
|
||||
validateString(message, 'message', {
|
||||
allowNewlines: true,
|
||||
modelFacing: true,
|
||||
});
|
||||
if (Buffer.byteLength(message, 'utf8') > LIMITS.maxMessageBytes) {
|
||||
throw new Error('message exceeds its byte limit');
|
||||
}
|
||||
@@ -325,6 +339,10 @@ function readFileBounded(fileDescriptor, maxBytes) {
|
||||
};
|
||||
}
|
||||
|
||||
function isRuleFileReadError(error) {
|
||||
return ['EACCES', 'EIO', 'EISDIR', 'EPERM'].includes(error?.code);
|
||||
}
|
||||
|
||||
function loadRuleFile({
|
||||
claudeDir,
|
||||
fileName,
|
||||
@@ -469,10 +487,12 @@ function loadRuleFile({
|
||||
diagnostic: null,
|
||||
bytesRead: consumedBytes,
|
||||
};
|
||||
} catch {
|
||||
} catch (error) {
|
||||
return {
|
||||
rule: null,
|
||||
diagnostic: diagnostic('HOOKIFY_RULE_INVALID', fileName, 'invalid rule schema or encoding'),
|
||||
diagnostic: isRuleFileReadError(error)
|
||||
? diagnostic('HOOKIFY_RULE_READ_FAILED', fileName, 'could not read rule file')
|
||||
: diagnostic('HOOKIFY_RULE_INVALID', fileName, 'invalid rule schema or encoding'),
|
||||
bytesRead: consumedBytes,
|
||||
};
|
||||
} finally {
|
||||
|
||||
@@ -8,6 +8,7 @@
|
||||
const { isMainThread, workerData } = require('worker_threads');
|
||||
|
||||
const HEADER_BYTES = Int32Array.BYTES_PER_ELEMENT * 2;
|
||||
const MAX_DIAGNOSTICS = 32;
|
||||
|
||||
function safeSource(value) {
|
||||
return typeof value === 'string' && /^hookify\.[A-Za-z0-9._-]+\.local\.md$/.test(value)
|
||||
@@ -34,7 +35,7 @@ function evaluateCondition(condition, values, diagnostics, source) {
|
||||
try {
|
||||
return new RegExp(condition.pattern, 'i').test(value);
|
||||
} catch {
|
||||
diagnostics.push({
|
||||
addDiagnostic(diagnostics, {
|
||||
code: 'HOOKIFY_REGEX_INVALID',
|
||||
message: `Hookify skipped ${safeSource(source)}: invalid regular expression.`,
|
||||
});
|
||||
@@ -45,6 +46,22 @@ function evaluateCondition(condition, values, diagnostics, source) {
|
||||
}
|
||||
}
|
||||
|
||||
function addDiagnostic(diagnostics, diagnostic) {
|
||||
if (diagnostics.length < MAX_DIAGNOSTICS) {
|
||||
diagnostics.push(diagnostic);
|
||||
return;
|
||||
}
|
||||
if (
|
||||
diagnostics.length === MAX_DIAGNOSTICS &&
|
||||
!diagnostics.some(item => item.code === 'HOOKIFY_DIAGNOSTICS_TRUNCATED')
|
||||
) {
|
||||
diagnostics.push({
|
||||
code: 'HOOKIFY_DIAGNOSTICS_TRUNCATED',
|
||||
message: 'Hookify skipped additional diagnostics because the diagnostic limit was reached.',
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
function evaluateTasks(tasks, values = {}) {
|
||||
const matchedIndexes = [];
|
||||
const diagnostics = [];
|
||||
@@ -105,5 +122,6 @@ if (!isMainThread) {
|
||||
|
||||
module.exports = {
|
||||
evaluateTasks,
|
||||
MAX_DIAGNOSTICS,
|
||||
writeResult,
|
||||
};
|
||||
|
||||
@@ -20,6 +20,9 @@ const LIMITS = Object.freeze({
|
||||
...LOADER_LIMITS,
|
||||
maxInputBytes: 256 * 1024,
|
||||
maxOutputBytes: 8192,
|
||||
maxContextBytes: 7000,
|
||||
maxContextBytesWithBlock: 3000,
|
||||
maxBlockReasonBytes: 3800,
|
||||
regexTimeoutMs: 250,
|
||||
});
|
||||
const EVENTS = new Set([
|
||||
@@ -216,9 +219,11 @@ function buildOutput(eventName, matches, diagnostics) {
|
||||
...warnings.map(formatRule),
|
||||
...formatDiagnostics(diagnostics),
|
||||
];
|
||||
const contextLimit = blocking.length > 0 ? 3000 : 7000;
|
||||
const contextLimit = blocking.length > 0
|
||||
? LIMITS.maxContextBytesWithBlock
|
||||
: LIMITS.maxContextBytes;
|
||||
const additionalContext = joinBounded(contextItems, contextLimit);
|
||||
const blockReason = joinBounded(blocking.map(formatRule), 3800);
|
||||
const blockReason = joinBounded(blocking.map(formatRule), LIMITS.maxBlockReasonBytes);
|
||||
|
||||
if (blocking.length === 0) {
|
||||
return contextOutput(eventName, additionalContext);
|
||||
|
||||
@@ -36,10 +36,11 @@ const SYNC_HOOKS = [
|
||||
matcher: '*',
|
||||
profiles: 'minimal,standard,strict',
|
||||
script: 'scripts/hooks/hookify-runner.js',
|
||||
run(raw) {
|
||||
run(raw, context = {}) {
|
||||
const result = runHookify(raw, {
|
||||
expectedEvent: 'PostToolUse',
|
||||
projectRoot: process.cwd()
|
||||
projectRoot: process.cwd(),
|
||||
truncated: context.truncated === true
|
||||
});
|
||||
return result.stdout === '{}' ? { ...result, stdout: '' } : result;
|
||||
}
|
||||
|
||||
@@ -5,6 +5,12 @@ description: This skill should be used when the user asks to create a hookify ru
|
||||
|
||||
# Writing Hookify Rules
|
||||
|
||||
## When to Activate
|
||||
|
||||
Use this skill when creating, reviewing, or debugging project-local Hookify
|
||||
rules, including `.claude/hookify.*.local.md` syntax, event selection,
|
||||
condition fields, rule limits, and warn/block behavior.
|
||||
|
||||
## Overview
|
||||
|
||||
Hookify rules are Markdown files with YAML frontmatter that define patterns to
|
||||
@@ -134,7 +140,14 @@ Match Claude Code's submitted `prompt` through the rule field `user_prompt`.
|
||||
|
||||
### Testing
|
||||
```bash
|
||||
node -e "console.log(new RegExp('your_pattern', 'i').test('test text'))"
|
||||
node - <<'NODE'
|
||||
const readline = require('node:readline/promises');
|
||||
const rl = readline.createInterface({ input: process.stdin, output: process.stdout });
|
||||
const pattern = await rl.question('pattern: ');
|
||||
const text = await rl.question('text: ');
|
||||
rl.close();
|
||||
console.log(new RegExp(pattern, 'i').test(text));
|
||||
NODE
|
||||
```
|
||||
|
||||
Regex evaluation runs in a resource-limited worker with one hard total
|
||||
|
||||
@@ -81,6 +81,15 @@ function runTests() {
|
||||
assert.ok(skill.includes('project `.claude/` directory'));
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('skill has activation guidance and a regex test command that does not embed user text in code', () => {
|
||||
const skill = read('skills/hookify-rules/SKILL.md');
|
||||
|
||||
assert.ok(skill.includes('## When to Activate'));
|
||||
assert.ok(skill.includes("readline.createInterface"));
|
||||
assert.ok(skill.includes("new RegExp(pattern, 'i')"));
|
||||
assert.ok(!skill.includes("new RegExp('your_pattern'"));
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('list and configure commands state that malformed rules are skipped rather than enforced', () => {
|
||||
const list = read('commands/hookify-list.md');
|
||||
const configure = read('commands/hookify-configure.md');
|
||||
|
||||
@@ -13,6 +13,7 @@ const {
|
||||
} = require('../../scripts/hooks/hookify-engine');
|
||||
const {
|
||||
evaluateTasks,
|
||||
MAX_DIAGNOSTICS,
|
||||
writeResult,
|
||||
} = require('../../scripts/hooks/hookify-regex-worker');
|
||||
|
||||
@@ -310,6 +311,29 @@ function runTests() {
|
||||
assert.ok(!result.diagnostics[0].message.includes('anything'));
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('caps invalid-regex diagnostics without discarding valid matches', () => {
|
||||
const input = {
|
||||
hook_event_name: 'PreToolUse',
|
||||
tool_name: 'Bash',
|
||||
tool_input: { command: 'npm publish' },
|
||||
};
|
||||
const invalidRules = Array.from(
|
||||
{ length: MAX_DIAGNOSTICS + 20 },
|
||||
(_value, index) => rule({
|
||||
name: `invalid-${index}`,
|
||||
source: `hookify.invalid-${index}.local.md`,
|
||||
conditions: [{ field: 'command', operator: 'regex_match', pattern: '(' }],
|
||||
})
|
||||
);
|
||||
const validRule = rule({ name: 'valid-regex' });
|
||||
|
||||
const result = evaluateRules([...invalidRules, validRule], input);
|
||||
|
||||
assert.deepStrictEqual(result.matches.map(item => item.name), ['valid-regex']);
|
||||
assert.ok(result.diagnostics.length <= MAX_DIAGNOSTICS + 1);
|
||||
assert.ok(result.diagnostics.some(item => item.code === 'HOOKIFY_DIAGNOSTICS_TRUNCATED'));
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('a regex timeout preserves an unrelated literal-only block match', () => {
|
||||
const input = {
|
||||
hook_event_name: 'PreToolUse',
|
||||
@@ -451,14 +475,24 @@ function runTests() {
|
||||
assert.ok(evaluated.diagnostics[0].message.includes('a Hookify rule'));
|
||||
|
||||
const shared = new SharedArrayBuffer(512);
|
||||
writeResult(shared, {
|
||||
matchedIndexes: [2],
|
||||
diagnostics: [],
|
||||
});
|
||||
let header = new Int32Array(shared, 0, 2);
|
||||
assert.strictEqual(Atomics.load(header, 0), 1);
|
||||
let bytes = new Uint8Array(shared, 8, Atomics.load(header, 1));
|
||||
let result = JSON.parse(Buffer.from(bytes).toString('utf8'));
|
||||
assert.deepStrictEqual(result.matchedIndexes, [2]);
|
||||
|
||||
writeResult(shared, {
|
||||
matchedIndexes: [],
|
||||
diagnostics: [{ code: 'X', message: 'x'.repeat(1000) }],
|
||||
});
|
||||
const header = new Int32Array(shared, 0, 2);
|
||||
header = new Int32Array(shared, 0, 2);
|
||||
assert.strictEqual(Atomics.load(header, 0), 2);
|
||||
const bytes = new Uint8Array(shared, 8, Atomics.load(header, 1));
|
||||
const result = JSON.parse(Buffer.from(bytes).toString('utf8'));
|
||||
bytes = new Uint8Array(shared, 8, Atomics.load(header, 1));
|
||||
result = JSON.parse(Buffer.from(bytes).toString('utf8'));
|
||||
assert.strictEqual(result.diagnostics[0].code, 'HOOKIFY_REGEX_WORKER_FAILED');
|
||||
})) passed++; else failed++;
|
||||
|
||||
|
||||
@@ -191,6 +191,39 @@ function runTests() {
|
||||
});
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('rejects condition list items after the conditions block is closed', () => {
|
||||
assert.throws(
|
||||
() => parseFrontmatter([
|
||||
'name: misplaced-condition',
|
||||
'enabled: true',
|
||||
'event: file',
|
||||
'conditions:',
|
||||
' - field: file_path',
|
||||
' operator: contains',
|
||||
' pattern: src/',
|
||||
'action: block',
|
||||
' - field: content',
|
||||
' operator: contains',
|
||||
' pattern: API_KEY',
|
||||
].join('\n')),
|
||||
/unsupported YAML structure/
|
||||
);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('rejects model-facing messages with bidi or invisible controls', () => {
|
||||
for (const message of ['Looks safe\u202E.gnirts', 'Invisible\uFEFFjoiner']) {
|
||||
assert.throws(
|
||||
() => validateRule({
|
||||
name: 'bidi-message',
|
||||
enabled: true,
|
||||
event: 'bash',
|
||||
pattern: 'deploy',
|
||||
}, message, 'hookify.bidi-message.local.md'),
|
||||
/message contains unsafe invisible or bidirectional characters/
|
||||
);
|
||||
}
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('rejects traversal names, symlinked files, and a symlinked .claude directory', () => {
|
||||
withProject(({ projectRoot, claudeDir }) => {
|
||||
const outside = path.join(projectRoot, 'outside.md');
|
||||
@@ -323,9 +356,20 @@ function runTests() {
|
||||
|
||||
const result = loadRules({ projectRoot, event: 'bash' });
|
||||
|
||||
assert.ok(result.rules.length <= LIMITS.maxRuleFiles);
|
||||
assert.strictEqual(result.rules.length, LIMITS.maxRuleFiles);
|
||||
assert.ok(result.totalBytes <= LIMITS.maxTotalBytes);
|
||||
assert.ok(result.diagnostics.some(item => item.code === 'HOOKIFY_RULE_LIMIT'));
|
||||
assert.ok(result.diagnostics.some(item =>
|
||||
item.code === 'HOOKIFY_RULE_LIMIT' &&
|
||||
item.message.includes('rule or directory entry count limit reached')
|
||||
));
|
||||
assert.ok(result.diagnostics.some(item =>
|
||||
item.code === 'HOOKIFY_RULE_LIMIT' &&
|
||||
item.message.includes('file exceeds byte limit')
|
||||
));
|
||||
assert.ok(result.diagnostics.some(item =>
|
||||
item.code === 'HOOKIFY_RULE_INVALID' &&
|
||||
item.message.includes('pattern-too-long')
|
||||
));
|
||||
});
|
||||
})) passed++; else failed++;
|
||||
|
||||
@@ -348,6 +392,45 @@ function runTests() {
|
||||
});
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('reports descriptor read failures separately from schema errors', () => {
|
||||
withProject(({ claudeDir }) => {
|
||||
const originalReadSync = fs.readSync;
|
||||
let readAttempted = false;
|
||||
writeRule(
|
||||
claudeDir,
|
||||
'hookify.read-fails.local.md',
|
||||
[
|
||||
'name: read-fails',
|
||||
'enabled: true',
|
||||
'event: bash',
|
||||
'pattern: safe',
|
||||
].join('\n')
|
||||
);
|
||||
|
||||
try {
|
||||
fs.readSync = function readSyncFails() {
|
||||
readAttempted = true;
|
||||
const error = new Error('sensitive device detail');
|
||||
error.code = 'EIO';
|
||||
throw error;
|
||||
};
|
||||
const result = loadRuleFile({
|
||||
claudeDir,
|
||||
fileName: 'hookify.read-fails.local.md',
|
||||
remainingTotalBytes: LIMITS.maxTotalBytes,
|
||||
expectedRealDirectory: fs.realpathSync(claudeDir),
|
||||
});
|
||||
|
||||
assert.strictEqual(readAttempted, true);
|
||||
assert.strictEqual(result.rule, null);
|
||||
assert.strictEqual(result.diagnostic.code, 'HOOKIFY_RULE_READ_FAILED');
|
||||
assert.ok(!result.diagnostic.message.includes('sensitive device detail'));
|
||||
} finally {
|
||||
fs.readSync = originalReadSync;
|
||||
}
|
||||
});
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('counts malformed file reads against the hard total byte budget', () => {
|
||||
withProject(({ projectRoot, claudeDir }) => {
|
||||
for (let index = 0; index < 12; index += 1) {
|
||||
|
||||
@@ -631,10 +631,17 @@ function runTests() {
|
||||
prompt: '',
|
||||
}],
|
||||
];
|
||||
for (const [hookId, _eventName, payload] of cases) {
|
||||
for (const [hookId, eventName, payload] of cases) {
|
||||
const output = run(JSON.stringify(payload), { projectRoot, hookId });
|
||||
assert.strictEqual(output.exitCode, 0);
|
||||
JSON.parse(output.stdout);
|
||||
|
||||
const mismatched = run(JSON.stringify({
|
||||
...payload,
|
||||
hook_event_name: eventName === 'Stop' ? 'PreToolUse' : 'Stop',
|
||||
}), { projectRoot, hookId });
|
||||
const serialized = JSON.stringify(JSON.parse(mismatched.stdout));
|
||||
assert.ok(serialized.includes('hook event did not match'));
|
||||
}
|
||||
const invalid = run(Buffer.from('not accepted'), {
|
||||
projectRoot,
|
||||
|
||||
@@ -444,6 +444,21 @@ function runTests() {
|
||||
});
|
||||
assert.strictEqual(blocked.warning, '');
|
||||
|
||||
const multiBlocked = mergeHookStdout([
|
||||
{
|
||||
id: 'post:test:block-one',
|
||||
stdout: JSON.stringify({ decision: 'block', reason: 'First reason.' }),
|
||||
},
|
||||
{
|
||||
id: 'post:test:block-two',
|
||||
stdout: JSON.stringify({ decision: 'block', reason: 'Second reason.' }),
|
||||
},
|
||||
]);
|
||||
assert.deepStrictEqual(JSON.parse(multiBlocked.stdout), {
|
||||
decision: 'block',
|
||||
reason: 'First reason.\n\nSecond reason.',
|
||||
});
|
||||
|
||||
const conflicting = mergeHookStdout([
|
||||
{ id: 'post:test:raw', stdout: 'plain output' },
|
||||
{ id: 'post:test:ctx', stdout: envelope('kept warning') }
|
||||
@@ -456,6 +471,32 @@ function runTests() {
|
||||
passed++;
|
||||
else failed++;
|
||||
|
||||
if (
|
||||
test('post:hookify receives dispatcher truncation state and fails open without pass-through', () => {
|
||||
const { SYNC_HOOKS, runHooks } = require(dispatcherPath);
|
||||
const hookify = SYNC_HOOKS.find(hook => hook.id === 'post:hookify');
|
||||
const result = runHooks(
|
||||
JSON.stringify({
|
||||
hook_event_name: 'PostToolUse',
|
||||
tool_name: 'Bash',
|
||||
tool_input: { command: 'true' },
|
||||
tool_response: {},
|
||||
}),
|
||||
[hookify],
|
||||
{ truncated: true, toolName: 'Bash' }
|
||||
);
|
||||
const parsed = JSON.parse(result.stdout);
|
||||
assert.ok(
|
||||
parsed.hookSpecificOutput.additionalContext.includes(
|
||||
'input exceeded the byte limit'
|
||||
)
|
||||
);
|
||||
assert.strictEqual(result.stderr, '');
|
||||
})
|
||||
)
|
||||
passed++;
|
||||
else failed++;
|
||||
|
||||
if (
|
||||
test('requiring the dispatcher module never dispatches; hooks.json calls cli()', () => {
|
||||
const raw = JSON.stringify({
|
||||
|
||||
Reference in New Issue
Block a user