fix(hooks): address review — close bypasses, surface failures, add test suite

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

@@ -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 <<EOF
 hooks:
   pre_tool_call:
     - matcher: "patch|write_file|edit|write|apply_patch|str_replace_editor"
-      command: "python3 /Users/YOU/.hermes/hooks/ecc-config-protection.py"
+      command: "python3 $HOOK_DIR/ecc-config-protection.py"
       timeout: 10
   post_tool_call:
     - matcher: "patch|write_file|edit|write|apply_patch|str_replace_editor"
-      command: "python3 /Users/YOU/.hermes/hooks/ecc-check-console-log.py"
+      command: "python3 $HOOK_DIR/ecc-check-console-log.py"
       timeout: 10
+EOF
 ```
 
 Restart Hermes (hooks are registered at session start), then approve the
@@ -64,14 +70,23 @@ hermes hooks list     # both hooks, with consent status
 hermes hooks doctor   # exec bit, allowlist, timing
 ```
 
-Payload-level proof (block case must print a JSON decision; allow cases stay silent):
+Payload-level proof — create a protected file first, so the block case has a
+real target (the hook only blocks edits to configs that already exist):
 
 ```bash
-echo '{"hook_event_name":"pre_tool_call","tool_name":"patch","tool_input":{"path":"/repo/eslint.config.mjs","old_string":"a","new_string":"b"}}' \
+T=$(mktemp -d) && touch "$T/eslint.config.mjs"
+echo "{\"hook_event_name\":\"pre_tool_call\",\"tool_name\":\"patch\",\"tool_input\":{\"path\":\"$T/eslint.config.mjs\",\"old_string\":\"a\",\"new_string\":\"b\"}}" \
   | python3 ~/.hermes/hooks/ecc-config-protection.py
 # {"decision": "block", "reason": "BLOCKED: modifying eslint.config.mjs ..."}
 ```
 
+Or run the committed suite, which exercises block, allow, case variants,
+dangling symlinks, multi-edit inputs, and fail-open diagnostics:
+
+```bash
+node tests/hermes-hooks.test.js
+```
+
 ## Notes
 
 - Match only the editing tools your Hermes config exposes; the shipped matcher

hooks/hermes/check-console-log.py

@@ -10,6 +10,9 @@ Contract (agent/shell_hooks.py):
   stdin : JSON payload {"hook_event_name":"post_tool_call","tool_name":"...",
                         "tool_input":{...},"tool_result":{...}}
   exit  : 0 (observer, no stdout JSON)
+
+Error handling: fail-open by design (observers must not brick the agent),
+but never silent — every swallowed path writes a one-line stderr diagnostic.
 """
 import json
 import os
@@ -20,11 +23,19 @@ EDIT_TOOLS = {"patch", "write_file", "edit", "write", "apply_patch", "str_replac
 EXCLUDE = re.compile(r"(\.d\.ts$|/__tests?__/|\.test\.[tj]sx?$|\.spec\.[tj]sx?$|/scripts/|console-safety)")
 
 
+def _warn(msg: str) -> 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

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
 
 

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);