diff --git a/CHANGELOG.md b/CHANGELOG.md index a5cec045..e4a14665 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Primitive hooks keep their fire limits across capt-hook releases.** + `nudge()`, `gate()`, and other primitives used the framework's versioned install path + as their state identity, resetting `max_fires` on each release. They now key on the + declaring pack or repo module. Loader hooks use `loader.py`, and review FIX routing, + `capt-hook list`, and hook lint see the correct hook files. Existing counters restart + once after upgrading; state stored under the old keys is not migrated. - **`capt-hook helper install` reports each step as it runs.** It used to capture every step and print nothing until the end, so a multi-minute `package-install` looked like a hang. The reason a step failed went only to the update log, and the error said just diff --git a/captain_hook/app.py b/captain_hook/app.py index 2d7cdace..a416a661 100644 --- a/captain_hook/app.py +++ b/captain_hook/app.py @@ -10,7 +10,7 @@ from typing import TYPE_CHECKING, Any, get_args from captain_hook.conditions import matches_conditions -from captain_hook.state import caller_file, hook_name +from captain_hook.state import caller_file, framework_frame, hook_name from captain_hook.types import ( TOOL_EVENTS, CustomCondition, @@ -300,6 +300,7 @@ def on( mandatory: bool = False, transcript_events: int | None = None, on_incomplete: str | None = None, + source_file: str | None = None, ) -> Callable[[HookHandler], HookHandler]: reject_async_decision(events, async_) reject_mandatory_misuse(events, async_, mandatory) @@ -324,12 +325,13 @@ def on( def decorator(fn: HookHandler) -> HookHandler: validate_handler_signature(fn) + defined = fn.__code__.co_filename _state.hooks.append( RegisteredHook( spec=spec, handler=fn, name=fn.__name__, - source_file=fn.__code__.co_filename, + source_file=source_file or (caller_file() if framework_frame(defined) else defined), ) ) return fn diff --git a/captain_hook/cli.py b/captain_hook/cli.py index 02dd8f92..d8604ada 100644 --- a/captain_hook/cli.py +++ b/captain_hook/cli.py @@ -31,6 +31,7 @@ register_fault_announcements, register_pr_announcements, register_resource_provisioning, + stamp_identity, ) from captain_hook.log import setup_logging from captain_hook.packs import manager, plugins @@ -122,12 +123,14 @@ def discover(self, *, scope: DiscoveryScope = "all") -> list[manager.ResolvedPac for pack_ in packs: discover_pack(pack_.name, pack_.path) register_pack_tools(packs) + before = len(_state.hooks) # Resource provisioning is one shared SessionStart hook; register it once with the union. if resources := list(dict.fromkeys(r for pack_ in packs for r in pack_.descriptor.resources)): register_resource_provisioning(resources) # The PR announcer's gating all lives in collect_announcements, so it registers unconditionally. register_pr_announcements() register_fault_announcements() + stamp_identity(before, root=Path(__file__).parent) return packs diff --git a/captain_hook/loader.py b/captain_hook/loader.py index 9d48b805..b1226ff0 100644 --- a/captain_hook/loader.py +++ b/captain_hook/loader.py @@ -131,7 +131,7 @@ def register_resource_provisioning(resources: Sequence[str]) -> None: so both event dispatch and settings generation see the async hook. """ - @on(Event.SessionStart, async_=True) + @on(Event.SessionStart, async_=True, source_file=__file__) def provision_pack_resources(evt: BaseHookEvent) -> None: from captain_hook.util.model_cache import provision_resources @@ -147,7 +147,7 @@ def register_pr_announcements() -> None: start tells the user what happened. """ - @on(Event.SessionStart, max_fires=1) + @on(Event.SessionStart, max_fires=1, source_file=__file__) def announce_pr_status(evt: BaseHookEvent) -> HookResult | None: from captain_hook.review.announce import collect_announcements from captain_hook.types import Action, HookResult @@ -166,7 +166,7 @@ def register_fault_announcements() -> None: reaches a person rather than only the daemon log. """ - @on(Event.SessionStart, max_fires=1) + @on(Event.SessionStart, max_fires=1, source_file=__file__) def announce_faults(evt: BaseHookEvent) -> HookResult | None: from captain_hook.faults import drain from captain_hook.types import Action, HookResult diff --git a/captain_hook/review/fix.py b/captain_hook/review/fix.py index ca85467f..9603e539 100644 --- a/captain_hook/review/fix.py +++ b/captain_hook/review/fix.py @@ -48,8 +48,7 @@ target verbatim (``repo`` ``None`` — fixed in place), but an installed-wheel or plugin-pack ``source_file`` carries no repo path, so the real hook comes from the decision ``kind``'s module prefix routed through the scan's :class:`~captain_hook.review.routing.PackIndex`. A -``nudge()``/``gate()`` fire records the primitive file, and a ``hook()`` bundled in a pack -records the wheel or plugin file — all of these route through the ``kind``: a +hook bundled in a pack records the wheel or plugin file, so it routes through the ``kind``: a ``.`` prefix naming a module a builtin pack ships targets the pack source inside captain-hook itself (``captain_hook/builtin_packs//hooks/.py``, repo captain-hook); a prefix naming a pack an enabled Claude Code plugin ships targets that plugin's ``plugin.json`` @@ -394,23 +393,23 @@ async def fire_message_target( def user_repo_source(source_file: str) -> bool: """A hook file living in the watched repo, not the installed wheel. - Installed-wheel source (a ``nudge()``/``gate()`` primitive fire, or a ``hook()`` bundled in a - builtin pack) carries ``captain_hook/`` in its path, so it resolves through the decision ``kind``'s - module prefix instead of being returned verbatim. Plugin-pack source is routed earlier. + Installed-wheel source (a hook bundled in a builtin pack) carries ``captain_hook/`` in its path, + so it resolves through the decision ``kind``'s module prefix instead of being returned verbatim. + Plugin-pack source is routed earlier. """ return f"{CAPTAIN_HOOK_ROOT}/" not in source_file -def plugin_rel_path(source_file: str, install_root: str) -> str | None: - """A plugin-pack ``source_file``'s path relative to its plugin install root, or ``None``. +def root_rel_path(source_file: str, root: str) -> str | None: + """A ``source_file``'s path relative to a plugin install root or the watched repo root, or ``None``. Best effort: a plugin rooted below its repo root (a ``plugin/`` subdir) carries that offset in the result, which the fix PR resolves against the plugin's repo. """ if not source_file: return None - source, root = Path(source_file).resolve(), Path(install_root).resolve() - return str(source.relative_to(root)) if source.is_relative_to(root) else None + source, base = Path(source_file).resolve(), Path(root).resolve() + return str(source.relative_to(base)) if source.is_relative_to(base) else None def plugin_route_for(decision: Decision, module: str, sep: str, index: PackIndex) -> PluginRoute | None: @@ -438,10 +437,11 @@ def resolve_target(decision: Decision, index: PackIndex) -> Target | None: if route.repo is None: logger.bind(hook=decision.kind).debug("dropped plugin-pack misfire; plugin declares no repository") return None - rel = plugin_rel_path(decision.source_file, route.root) + rel = root_rel_path(decision.source_file, route.root) return Target(rel or decision.source_file, decision.kind, route.repo, module.split(".")[0] if sep else None) if user_repo_source(decision.source_file): - return Target(decision.source_file, decision.kind, repo=None, pack=None) + rel = root_rel_path(decision.source_file, str(index.root)) if index.root else None + return Target(rel or decision.source_file, decision.kind, repo=None, pack=None) if not sep or not module: return None match module.split("."): diff --git a/captain_hook/review/routing.py b/captain_hook/review/routing.py index 6e3dc1fa..abf8b823 100644 --- a/captain_hook/review/routing.py +++ b/captain_hook/review/routing.py @@ -74,11 +74,13 @@ class PackIndex: to its :class:`PluginRoute`. Membership marks a kind as a plugin-pack misfire. plugin_dirs: The ``hooks/`` dir of each plugin pack mapped to its route — the source arm for a hook fired under a bare ``@on`` name (no ``.`` prefix in its ``kind``). + root: The watched repo's root, which a repo-local hook's ``source_file`` relativizes against. """ builtins: Mapping[str, Path] plugin_prefixes: Mapping[str, PluginRoute] = field(default_factory=dict) plugin_dirs: Mapping[str, PluginRoute] = field(default_factory=dict) + root: Path | None = None @classmethod def load(cls, root: Path | None) -> PackIndex: @@ -107,4 +109,4 @@ def load(cls, root: Path | None) -> PackIndex: route = PluginRoute(RepoKey(normalize_origin(repository)) if repository else None, plugin.root) prefixes[manager.pack_module_name(plugin.id)] = route dirs[str(plugin_discovery.plugin_pack_root(plugin) / manager.HOOKS_DIRNAME)] = route - return cls(builtins=builtins, plugin_prefixes=prefixes, plugin_dirs=dirs) + return cls(builtins=builtins, plugin_prefixes=prefixes, plugin_dirs=dirs, root=root) diff --git a/tests/test_decisions_writer.py b/tests/test_decisions_writer.py index bd127c67..22a38373 100644 --- a/tests/test_decisions_writer.py +++ b/tests/test_decisions_writer.py @@ -161,7 +161,7 @@ def test_primitive_built_hook_keeps_source_pair(self, db_path: Path, tmp_path: P (row,) = rows(db_path) assert row.kind.split(":")[-1].startswith("nudge") - assert row.source_file.endswith("captain_hook/primitives/nudge.py") + assert row.source_file == __file__ assert (row.event, row.action, row.message) == ("PreToolUse", "warn", "Remember to run tests") assert row.tool_name == "Edit" assert row.tool_digest == evt.tool_digest diff --git a/tests/test_pack_imports.py b/tests/test_pack_imports.py index 7ba786f0..83f4d4ab 100644 --- a/tests/test_pack_imports.py +++ b/tests/test_pack_imports.py @@ -1,10 +1,14 @@ from __future__ import annotations +import json +import os +import subprocess import sys from pathlib import Path import pytest +import captain_hook from captain_hook import app from captain_hook.loader import discover_pack @@ -12,6 +16,24 @@ "from ._common import SHARED\nfrom captain_hook import Event, hook\nhook(Event.PreToolUse, message=str(SHARED))\n" ) ON_HANDLER_SRC = "from captain_hook import Event, on\n\n\n@on(Event.PostToolUse)\ndef check(evt):\n return None\n" +PRIMITIVES_SRC = ( + "from captain_hook import gate, llm_nudge, nudge, rewrite_command, set_tool_input\n" + "nudge('Remember the release notes')\n" + "gate('Run the suite before stopping')\n" + "llm_nudge('Is the agent guessing?', message='Observe first')\n" + "set_tool_input('model', 'sonnet', tool='Agent')\n" + "rewrite_command('cat $$$ARGS', 'bat $$$ARGS')\n" +) +INSTALLED_PROBE = ( + "import json, sys\n" + "from pathlib import Path\n" + "import captain_hook\n" + "from captain_hook.app import _state\n" + "from captain_hook.loader import discover_pack\n" + "discover_pack('ccx', Path(sys.argv[1]))\n" + "print(json.dumps({'package': captain_hook.__file__," + " 'hooks': [[h.state_key, h._state_identity, h.source_file] for h in _state.hooks]}))\n" +) @pytest.fixture @@ -91,6 +113,58 @@ def test_state_key_stable_across_pack_root_moves(tmp_path: Path, isolate_modules assert key_a == key_b +def installed_state_keys(install: Path, pack: Path) -> list[list[str]]: + install.mkdir() + (install / "captain_hook").symlink_to(Path(captain_hook.__file__).parent, target_is_directory=True) + probe = subprocess.run( + [sys.executable, "-c", INSTALLED_PROBE, str(pack)], + cwd=install, + env={**os.environ, "PYTHONPATH": str(install)}, + capture_output=True, + text=True, + check=True, + ) + loaded = json.loads(probe.stdout) + assert loaded["package"] == str(install / "captain_hook" / "__init__.py") + return loaded["hooks"] + + +def test_primitive_state_keys_stable_across_capt_hook_installs(tmp_path: Path) -> None: + pack = _pack_root(tmp_path / "pack", PRIMITIVES_SRC) + + first = installed_state_keys(tmp_path / "v1", pack) + second = installed_state_keys(tmp_path / "v2", pack) + + assert len(first) == 5 + assert first == second + assert {(identity, source) for _, identity, source in first} == {("ccx\0guard.py", str(pack / "guard.py"))} + + +def test_framework_hooks_key_on_their_package_relative_path(tmp_path: Path, isolate_modules: None) -> None: + from captain_hook.cli import CliState + + (hooks := tmp_path / ".claude" / "hooks").mkdir(parents=True) + CliState(root=tmp_path, hooks=str(hooks)).discover(scope="all") + + framework = {"announce_pr_status", "announce_faults"} + assert {h._state_identity for h in app._state.hooks if h.name in framework} == {"loader.py"} + + +def test_shared_decorator_keeps_each_handler_file(tmp_path: Path, isolate_modules: None) -> None: + pack = tmp_path / "pack" + pack.mkdir() + (pack / "_shared.py").write_text( + "from captain_hook import Event, on\n\npost = on(Event.PostToolUse, max_fires=1)\n" + ) + for stem in ("alpha", "beta"): + (pack / f"{stem}.py").write_text("from ._shared import post\n\n\n@post\ndef check(evt):\n return None\n") + + app.reset() + discover_pack("ccx", pack) + + assert sorted(h._state_identity for h in app._state.hooks) == ["ccx\0alpha.py", "ccx\0beta.py"] + + def test_state_key_differs_for_two_packs_same_hook_name(tmp_path: Path, isolate_modules: None) -> None: # Two packs each register an @on handler named "check"; the pack name in the identity keeps # their state keys (and thus max_fires counters) distinct. diff --git a/tests/test_review_fix.py b/tests/test_review_fix.py index a8530bb7..1dea2c35 100644 --- a/tests/test_review_fix.py +++ b/tests/test_review_fix.py @@ -454,6 +454,11 @@ class TestResolveTarget: def test_resolve_target(self, source_file: str, kind: str, expected: Target | None) -> None: assert resolve_target(make_decision(kind=kind, source_file=source_file), INDEX) == expected + def test_repo_hook_relativizes_against_the_watched_repo(self, tmp_path: Path) -> None: + index = PackIndex(builtins=INDEX.builtins, root=tmp_path) + decision = make_decision(kind="guard:nudge_deadbeef", source_file=str(tmp_path / ".claude/hooks/guard.py")) + assert resolve_target(decision, index) == Target(".claude/hooks/guard.py", "guard:nudge_deadbeef", None, None) + def test_plugin_pack_routes_to_its_repository(self) -> None: index = PackIndex(builtins=INDEX.builtins, plugin_prefixes={NOTIFY_PREFIX: NOTIFY_ROUTE}) target = resolve_target(make_decision(kind=NOTIFY_KIND, source_file=NOTIFY_SOURCE), index)