diff --git a/scripts/check_install_effective.py b/scripts/check_install_effective.py index 11894d0e..5642d55c 100755 --- a/scripts/check_install_effective.py +++ b/scripts/check_install_effective.py @@ -33,6 +33,7 @@ """ from __future__ import annotations +import json import os import pwd import re @@ -155,6 +156,8 @@ def check_links() -> list[str]: name = skill.parent.name installed = HOME / ".claude/skills" / name if installed.exists() and not installed.is_symlink(): + if installed.is_dir() and (installed / ".catstack-generated").is_file(): + continue problems.append(f"skill shadowed by a real directory: {installed}") return problems @@ -226,17 +229,58 @@ def check_worktree_links() -> tuple[list[str], list[str]]: return problems, unchecked +def hook_commands_by_event(data: object) -> dict[str, set[str]]: + if not isinstance(data, dict): + return {} + hooks = data.get("hooks", {}) + if not isinstance(hooks, dict): + return {} + commands_by_event: dict[str, set[str]] = {} + for event, entries in hooks.items(): + if not isinstance(event, str) or not isinstance(entries, list): + continue + commands = commands_by_event.setdefault(event, set()) + for entry in entries: + if not isinstance(entry, dict): + continue + entry_hooks = entry.get("hooks", []) + if not isinstance(entry_hooks, list): + continue + for hook in entry_hooks: + if not isinstance(hook, dict): + continue + command = hook.get("command") + if isinstance(command, str): + commands.add(command) + return commands_by_event + + def check_hooks_registered() -> list[str]: settings = HOME / ".claude/settings.json" if not settings.exists(): return ["no ~/.claude/settings.json; no hook is registered"] - text = settings.read_text() + try: + settings_data = json.loads(settings.read_text(encoding="utf-8")) + except OSError as exc: + return [f"UNCHECKED: ~/.claude/settings.json could not be read ({exc.__class__.__name__}); hooks are unchecked"] + except json.JSONDecodeError as exc: + return [ + f"UNCHECKED: ~/.claude/settings.json could not be parsed as JSON " + f"({exc.msg} at line {exc.lineno}, column {exc.colno}); hooks are unchecked" + ] + registered = hook_commands_by_event(settings_data) problems = [] - for hook_dir in sorted((REPO / "engine/hooks").glob("*/")): - if not (hook_dir / "claude.hook.json").exists(): - continue - if hook_dir.name not in text: - problems.append(f"hook built but never registered in settings.json: {hook_dir.name}") + for hook_file in sorted((REPO / "engine/hooks").glob("*/claude*.hook.json")): + hook_dir = hook_file.parent + with hook_file.open(encoding="utf-8") as handle: + hook_data = json.load(handle) + for event, commands in hook_commands_by_event(hook_data).items(): + for command in sorted(commands): + if command not in registered.get(event, set()): + problems.append( + f"hook declared but not registered for {event} in settings.json: " + f"{hook_dir.name}/{hook_file.name}: {command}" + ) return problems diff --git a/tests/test_install_effective.py b/tests/test_install_effective.py index 54629305..ac60e898 100644 --- a/tests/test_install_effective.py +++ b/tests/test_install_effective.py @@ -13,6 +13,7 @@ import contextlib import importlib.util import io +import json import os import pwd import shutil @@ -106,6 +107,24 @@ def run_installed_checker(repo, home): return code, out.getvalue() +def hook_entry(command): + return [{"matcher": "", "hooks": [{"type": "command", "command": command}]}] + + +def write_settings(home, hooks): + (Path(home) / ".claude/settings.json").write_text(json.dumps({"hooks": hooks}), encoding="utf-8") + + +def write_declared_hook(repo, event="UserPromptSubmit", command="$HOME/.claude/hooks/demo-freeze/run.py"): + hook = Path(repo) / "engine/hooks/demo-freeze" + hook.mkdir(parents=True) + (hook / "claude.hook.json").write_text( + json.dumps({"hooks": {event: hook_entry(command)}}), + encoding="utf-8", + ) + return "demo-freeze", event, command + + class TestSandboxHomeIsSkippedNotFailed(unittest.TestCase): def test_a_throwaway_home_is_named_as_a_sandbox(self): with tempfile.TemporaryDirectory() as home: @@ -218,15 +237,61 @@ def test_links_resolving_into_the_primary_checkout_pass(self): self.assertEqual(code, 0, output) self.assertNotIn(WORKTREE_NAME, output) - def test_an_unregistered_hook_still_fails_when_no_link_is_in_a_worktree(self): + def test_a_declared_hook_command_under_the_wrong_event_is_reported(self): + with tempfile.TemporaryDirectory() as tmp: + repo, home = build_installation(tmp, link_into_worktree=False) + hook_name, event, command = write_declared_hook(repo) + write_settings(home, {"Stop": hook_entry(command)}) + code, output = run_installed_checker(repo, home) + self.assertEqual(code, 1, output) + self.assertIn(hook_name, output) + self.assertIn("claude.hook.json", output) + self.assertIn(event, output) + self.assertIn(command, output) + + def test_a_declared_hook_event_with_the_wrong_command_is_reported(self): + with tempfile.TemporaryDirectory() as tmp: + repo, home = build_installation(tmp, link_into_worktree=False) + hook_name, event, command = write_declared_hook(repo) + write_settings(home, {event: hook_entry("$HOME/.claude/hooks/demo-freeze/other.py")}) + code, output = run_installed_checker(repo, home) + self.assertEqual(code, 1, output) + self.assertIn(hook_name, output) + self.assertIn("claude.hook.json", output) + self.assertIn(event, output) + self.assertIn(command, output) + + def test_a_declared_hook_event_and_command_pair_is_silent(self): + with tempfile.TemporaryDirectory() as tmp: + repo, home = build_installation(tmp, link_into_worktree=False) + hook_name, event, command = write_declared_hook(repo) + write_settings(home, {event: hook_entry(command)}) + code, output = run_installed_checker(repo, home) + self.assertEqual(code, 0, output) + self.assertNotIn(hook_name, output) + self.assertNotIn(command, output) + + def test_an_installer_generated_skill_directory_is_silent(self): + with tempfile.TemporaryDirectory() as tmp: + repo, home = build_installation(tmp, link_into_worktree=False) + installed = Path(home) / ".claude/skills/cat-mode" + installed.unlink() + installed.mkdir() + (installed / ".catstack-generated").write_text("", encoding="utf-8") + code, output = run_installed_checker(repo, home) + self.assertEqual(code, 0, output) + self.assertNotIn("skill shadowed", output) + + def test_a_hand_made_skill_directory_is_reported(self): with tempfile.TemporaryDirectory() as tmp: repo, home = build_installation(tmp, link_into_worktree=False) - hook = repo / "engine/hooks/demo-freeze" - hook.mkdir(parents=True) - (hook / "claude.hook.json").write_text("{}", encoding="utf-8") + installed = Path(home) / ".claude/skills/cat-mode" + installed.unlink() + installed.mkdir() code, output = run_installed_checker(repo, home) self.assertEqual(code, 1, output) - self.assertIn("hook built but never registered", output) + self.assertIn("skill shadowed by a real directory", output) + self.assertIn(str(installed), output) def relink_claude_md(home, source):