From b30a8c782a1b85673d07c54dec09854ff7c7c7a2 Mon Sep 17 00:00:00 2001 From: ravitejarenangi Date: Tue, 8 Sep 2026 12:38:29 +0530 Subject: [PATCH] =?UTF-8?q?fix(hooks):=20address=20review=20=E2=80=94=20cl?= =?UTF-8?q?ose=20bypasses,=20surface=20failures,=20add=20test=20suite?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile + CodeRabbit review of #3019: - config-protection: compare basenames case-insensitively (macOS/Windows resolve ESLINT.CONFIG.JS to the protected eslint.config.js) and use lstat so a dangling symlink to a protected name still blocks (os.path.exists follows the link and returns False). - Both hooks: fail-open stays, silence goes — every swallowed path now writes a one-line stderr diagnostic. - config-protection: sys.stdout.write instead of print() per repo Python style; block decisions carry a trailing newline. - New tests/hermes-hooks.test.js: 13 cases through the real scripts via spawnSync — block (all names, case variants, dangling symlink, edits[], per-tool), allow (normal source, new config, non-edit tools), fail-open with diagnostics (malformed payload, missing path, unreadable file). - docs: absolute-path copy-paste block via $HOME (CodeRabbit: /Users/YOU placeholders were unverifiable), self-contained block-case proof that creates its target, pointer to the committed suite. --- docs/HERMES-HOOKS.md | 25 +++- hooks/hermes/check-console-log.py | 31 +++- hooks/hermes/config-protection.py | 59 ++++++-- tests/hermes-hooks.test.js | 241 ++++++++++++++++++++++++++++++ 4 files changed, 330 insertions(+), 26 deletions(-) create mode 100644 tests/hermes-hooks.test.js diff --git a/docs/HERMES-HOOKS.md b/docs/HERMES-HOOKS.md index 0ced4ea77..0c7570650 100644 --- a/docs/HERMES-HOOKS.md +++ b/docs/HERMES-HOOKS.md @@ -37,16 +37,22 @@ cp hooks/hermes/config-protection.py ~/.hermes/hooks/ecc-config-protection.py cp hooks/hermes/check-console-log.py ~/.hermes/hooks/ecc-check-console-log.py chmod +x ~/.hermes/hooks/ecc-*.py -# 2. Append to ~/.hermes/config.yaml (Hermes wants the mapping keyed by event): +# 2. Point the commands at the copies you just made. Substitute YOUR absolute +# home path — Hermes resolves hook commands verbatim, without ~ expansion. +HOOK_DIR="$HOME/.hermes/hooks" + +# 3. Append to ~/.hermes/config.yaml (Hermes wants the mapping keyed by event): +cat >> ~/.hermes/config.yaml < None: + sys.stderr.write(f"[ecc-check-console-log] {msg}\n") + + def main() -> int: + raw = sys.stdin.read() try: - raw = sys.stdin.read() payload = json.loads(raw) if raw.strip() else {} - except Exception: + if not isinstance(payload, dict): + _warn(f"payload is {type(payload).__name__}, not an object — skipping") + return 0 + except ValueError as exc: + _warn(f"unparseable payload ({exc}) — skipping") return 0 tool = str(payload.get("tool_name") or "") @@ -32,26 +43,34 @@ def main() -> int: return 0 ti = payload.get("tool_input") or {} + if not isinstance(ti, dict): + _warn("tool_input is not an object — skipping") + return 0 path = None for key in ("path", "file_path", "filePath", "target"): v = ti.get(key) if isinstance(v, str) and v.strip(): path = v.strip() break - if not path or not os.path.isfile(path): + if not path: + _warn(f"no path found in tool_input for tool {tool!r} — skipping") return 0 if not re.search(r"\.[tj]sx?$", path) or EXCLUDE.search(path): return 0 + if not os.path.isfile(path): + _warn(f"{path} is not a regular file on disk — skipping") + return 0 try: with open(path, "r", errors="replace") as f: content = f.read() - except OSError: + except OSError as exc: + _warn(f"could not read {path} ({exc.strerror or exc}) — skipping") return 0 if "console.log(" in content: - sys.stderr.write( - f"[hook] console.log() present in {path} — remove it before committing " + _warn( + f"console.log() present in {path} — remove it before committing " "(use the project logger instead)." ) return 0 diff --git a/hooks/hermes/config-protection.py b/hooks/hermes/config-protection.py index 8e68a222d..31315f840 100755 --- a/hooks/hermes/config-protection.py +++ b/hooks/hermes/config-protection.py @@ -14,6 +14,10 @@ Contract (agent/shell_hooks.py): Blocks ONLY file-editing tools, ONLY when the target already exists on disk. Creating a config from scratch is allowed (new project), and so is reading. + +Error handling: the hook is fail-open BY DESIGN (a broken hook must not +brick the agent), but it never fails silently — every swallowed path writes +a one-line diagnostic to stderr. """ import json import os @@ -35,11 +39,19 @@ PROTECTED = { EDIT_TOOLS = {"patch", "write_file", "edit", "write", "apply_patch", "str_replace_editor"} +def _warn(msg: str) -> None: + sys.stderr.write(f"[ecc-config-protection] {msg}\n") + + def main() -> int: + raw = sys.stdin.read() try: - raw = sys.stdin.read() payload = json.loads(raw) if raw.strip() else {} - except Exception: + if not isinstance(payload, dict): + _warn(f"payload is {type(payload).__name__}, not an object — allowing") + return 0 + except ValueError as exc: + _warn(f"unparseable payload ({exc}) — allowing") return 0 # unparseable payload never blocks tool = str(payload.get("tool_name") or "") @@ -47,6 +59,9 @@ def main() -> int: return 0 ti = payload.get("tool_input") or {} + if not isinstance(ti, dict): + _warn("tool_input is not an object — allowing") + return 0 # Every Hermes/CLI editing tool puts the target somewhere obvious. candidates = [] for key in ("path", "file_path", "filePath", "target", "filename", "notebook_path"): @@ -63,24 +78,38 @@ def main() -> int: if isinstance(v, str) and v.strip(): candidates.append(v.strip()) if not candidates: + _warn(f"no path found in tool_input for tool {tool!r} — allowing") return 0 for p in candidates: + # Case-insensitive filesystems (macOS default, Windows) resolve + # ESLINT.CONFIG.JS to the protected eslint.config.js; compare the + # basename lowercased so a case variant cannot slip past. base = os.path.basename(p) - if base in PROTECTED and os.path.exists(p): - print( - json.dumps( - { - "decision": "block", - "reason": ( - f"BLOCKED: modifying {base} is not allowed. Fix the source code so the " - "linter/formatter passes instead of weakening the config. For a legitimate " - "config change, ask the owner to edit it directly." - ), - } + if base.lower() in PROTECTED: + # os.path.exists() is False for a dangling symlink, yet editing + # tools happily follow/replace the link target — use lstat so a + # dangling symlink to a protected name still counts as existing. + try: + os.lstat(p) + exists = True + except OSError: + exists = False + if exists: + sys.stdout.write( + json.dumps( + { + "decision": "block", + "reason": ( + f"BLOCKED: modifying {base} is not allowed. Fix the source code so the " + "linter/formatter passes instead of weakening the config. For a legitimate " + "config change, ask the owner to edit it directly." + ), + } + ) + + "\n" ) - ) - return 0 + return 0 return 0 diff --git a/tests/hermes-hooks.test.js b/tests/hermes-hooks.test.js new file mode 100644 index 000000000..64d8cb3d1 --- /dev/null +++ b/tests/hermes-hooks.test.js @@ -0,0 +1,241 @@ +/** + * Tests for hooks/hermes/ — the Hermes-native config-protection and + * console-log guard hooks. + * + * Run standalone: node tests/hermes-hooks.test.js + * Picked up automatically by tests/run-all.js (glob: tests/ star star /.test.js). + * + * Each case feeds a REAL payload through the actual script via spawnSync — + * same stdin/stdout contract Hermes uses — and asserts on the emitted JSON + * or stderr. Fail-open paths additionally assert the diagnostic line, so a + * regression cannot silently disable the policy. + */ + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const repoRoot = path.resolve(__dirname, '..'); +const GUARD = path.join(repoRoot, 'hooks', 'hermes', 'config-protection.py'); +const LOGGER = path.join(repoRoot, 'hooks', 'hermes', 'check-console-log.py'); + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + return true; + } catch (err) { + console.log(` ✗ ${name}`); + console.log(` Error: ${err.message}`); + return false; + } +} + +/** Run a hook with a payload; return {rc, stdout, stderr} and assert clean spawn. */ +function runHook(script, payload) { + const res = spawnSync('python3', [script], { + input: JSON.stringify(payload), + encoding: 'utf8', + timeout: 15000, + }); + assert.strictEqual(res.error, undefined, `spawn failed: ${res.error}`); + assert.strictEqual(res.status, 0, `hook must always exit 0 (got ${res.status})`); + return { stdout: res.stdout, stderr: res.stderr }; +} + +function decision(stdout) { + const text = (stdout || '').trim(); + if (!text) return null; + const parsed = JSON.parse(text); + assert.ok( + parsed.decision === 'block' && typeof parsed.reason === 'string' && parsed.reason.length > 0, + `block decision must carry a reason, got: ${text}` + ); + return parsed; +} + +// ─── config-protection: blocking ─────────────────────────────────────────── + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-hermes-hooks-')); +const protectedConfigs = [ + 'eslint.config.mjs', + '.eslintrc.json', + 'prettier.config.js', + 'biome.json', + 'ruff.toml', + 'lint-ratchet-baseline.json', +]; + +let passed = 0; +let failed = 0; +const results = []; + +function run(name, fn) { + const ok = test(name, fn); + if (ok) passed += 1; else failed += 1; +} + +run('blocks edits to every protected config name', () => { + for (const name of protectedConfigs) { + const target = path.join(tmp, name); + fs.writeFileSync(target, '{}\n'); + const { stdout } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: 'patch', + tool_input: { path: target, old_string: 'a', new_string: 'b' }, + }); + const d = decision(stdout); + assert.ok(d.reason.includes(name), `reason should name ${name}`); + } +}); + +run('blocks case-variant names (case-insensitive filesystems)', () => { + const target = path.join(tmp, 'ESLINT.CONFIG.MJS'); + fs.writeFileSync(path.join(tmp, 'eslint.config.mjs'), 'export default {}\n'); + // On a case-sensitive filesystem the variant is a distinct name; the hook + // must still refuse it so a macOS/Windows rename cannot smuggle an edit. + const { stdout } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: 'patch', + tool_input: { path: target, old_string: 'a', new_string: 'b' }, + }); + assert.ok(decision(stdout), 'case variant of a protected name must block'); +}); + +run('blocks a dangling symlink to a protected name', () => { + const dangling = path.join(tmp, 'eslint.config.cjs'); + try { fs.unlinkSync(dangling); } catch { /* not present yet */ } + fs.symlinkSync(path.join(tmp, 'does-not-exist-at-all'), dangling); + // Sanity: the link itself exists (lstat succeeds) but resolves nowhere — + // os.path.exists() is False here, which is exactly the bypass being pinned. + assert.ok(fs.lstatSync(dangling).isSymbolicLink(), 'symlink must exist for the case to be real'); + assert.ok(!fs.existsSync(dangling), 'link must dangle for the case to be real'); + const { stdout } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: 'patch', + tool_input: { path: dangling, old_string: 'a', new_string: 'b' }, + }); + assert.ok(decision(stdout), 'dangling symlink to protected name must block'); +}); + +run('blocks paths under edits[] (multi-edit inputs)', () => { + const target = path.join(tmp, '.eslintrc'); + fs.writeFileSync(target, '{}\n'); + const { stdout } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: 'str_replace_editor', + tool_input: { edits: [{ old_string: 'a', new_string: 'b', file_path: target }] }, + }); + assert.ok(decision(stdout), 'nested edits[] path must block'); +}); + +run('blocks for every recognised editing tool', () => { + const target = path.join(tmp, 'biome.json'); + fs.writeFileSync(target, '{}\n'); + for (const tool of ['patch', 'write_file', 'edit', 'write', 'apply_patch', 'str_replace_editor']) { + const { stdout } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: tool, + tool_input: { path: target }, + }); + assert.ok(decision(stdout), `${tool} must be gated`); + } +}); + +// ─── config-protection: allowing ─────────────────────────────────────────── + +run('allows edits to normal source files', () => { + const target = path.join(tmp, 'src.tsx'); + fs.writeFileSync(target, 'export const x = 1;\n'); + const { stdout, stderr } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: 'patch', + tool_input: { path: target, old_string: '1', new_string: '2' }, + }); + assert.strictEqual(stdout.trim(), '', 'normal edits emit nothing'); + assert.strictEqual(stderr.trim(), '', 'and stay silent'); +}); + +run('allows creating a protected config that does not exist yet', () => { + const { stdout } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: 'write_file', + tool_input: { path: path.join(tmp, 'fresh-project', '.eslintrc.json'), content: '{}' }, + }); + assert.strictEqual(stdout.trim(), '', 'first-time config creation is allowed'); +}); + +run('allows non-editing tools (read/search)', () => { + const target = path.join(tmp, '.eslintrc'); + for (const tool of ['read_file', 'search_files', 'terminal']) { + const { stdout } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: tool, + tool_input: { path: target, pattern: 'x' }, + }); + assert.strictEqual(stdout.trim(), '', `${tool} is never gated`); + } +}); + +// ─── config-protection: fail-open + diagnostics ──────────────────────────── + +run('malformed payload: allows AND reports on stderr', () => { + const res = spawnSync('python3', [GUARD], { input: 'not-json{{', encoding: 'utf8', timeout: 15000 }); + assert.strictEqual(res.status, 0, 'must exit 0 (fail-open)'); + assert.strictEqual(res.stdout.trim(), '', 'no decision emitted'); + assert.ok(res.stderr.includes('unparseable payload'), 'diagnostic required'); +}); + +run('missing path in tool_input: allows AND reports', () => { + const { stdout, stderr } = runHook(GUARD, { + hook_event_name: 'pre_tool_call', + tool_name: 'patch', + tool_input: { old_string: 'a', new_string: 'b' }, + }); + assert.strictEqual(stdout.trim(), ''); + assert.ok(stderr.includes('no path found'), 'diagnostic required'); +}); + +// ─── check-console-log ───────────────────────────────────────────────────── + +run('warns when an edited file contains console.log()', () => { + const target = path.join(tmp, 'noisy.ts'); + fs.writeFileSync(target, 'export function f(){ console.log("x"); }\n'); + const { stdout, stderr } = runHook(LOGGER, { + hook_event_name: 'post_tool_call', + tool_name: 'write_file', + tool_input: { path: target, content: 'x' }, + }); + assert.strictEqual(stdout.trim(), '', 'observer emits no stdout JSON'); + assert.ok(stderr.includes('console.log() present'), 'warning required'); +}); + +run('stays silent for clean files, tests, and non-edit tools', () => { + const clean = path.join(tmp, 'clean.ts'); + fs.writeFileSync(clean, 'export function g(){ return 1; }\n'); + const spec = path.join(tmp, 'thing.spec.ts'); + fs.writeFileSync(spec, 'console.log("in a spec");\n'); + for (const [tool, p] of [['write_file', clean], ['write_file', spec], ['read_file', clean]]) { + const { stdout, stderr } = runHook(LOGGER, { + hook_event_name: 'post_tool_call', + tool_name: tool, + tool_input: { path: p }, + }); + assert.strictEqual(stdout.trim(), '', 'no stdout'); + assert.ok(!stderr.includes('console.log() present'), `${p} must not warn`); + } +}); + +run('unreadable file: skips AND reports on stderr', () => { + const { stderr } = runHook(LOGGER, { + hook_event_name: 'post_tool_call', + tool_name: 'write_file', + tool_input: { path: path.join(tmp, 'not-on-disk.ts') }, + }); + assert.ok(/not a regular file|skipping/.test(stderr), 'diagnostic required'); +}); + +console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); +if (failed > 0) process.exit(1);