diff --git a/engine/hooks/_runner/dispatch.py b/engine/hooks/_runner/dispatch.py index 7f1dd37f2..35743450d 100644 --- a/engine/hooks/_runner/dispatch.py +++ b/engine/hooks/_runner/dispatch.py @@ -1,11 +1,12 @@ from __future__ import annotations import argparse -import concurrent.futures import glob +import importlib.util import json import os import re +import signal import subprocess import sys import time @@ -16,7 +17,11 @@ import run DEFAULT_BUDGET = 59.5 +DEFAULT_SUBAGENT_STOP_EVENT_BUDGET = 9.0 +DEFAULT_SUBAGENT_STOP_DETECTOR_BUDGET = 4.0 OPT_OUT_KEY = "subagent_stop" +DISPATCH_KEY = "dispatch" +DEFAULT_DISPATCH_ENTRY = "detect:detect" MIRRORED_EVENTS = {"SubagentStop": "Stop"} DIRECT_RE = re.compile(r"^python3 \$HOME/\.(claude|cursor|codex)/hooks/([^/\s]+)/([^/\s]+\.py)((?:\s+.*)?)$") MESSAGE_KEYS = ("reason", "message", "additionalContext", "additional_context", "user_message") @@ -157,6 +162,11 @@ def _load_manifest_hooks( opt_out = manifest.get(OPT_OUT_KEY) if isinstance(opt_out, dict) and opt_out.get("inherit") is False: continue + dispatch_entry = _dispatch_entry(manifest) + if dispatch_entry is None: + continue + else: + dispatch_entry = None for entry in entries: matcher = None if not mirrored and isinstance(entry, dict): @@ -178,11 +188,26 @@ def _load_manifest_hooks( "args": list(args), "timeout": hook.get("timeout"), "matcher": matcher, + "dispatch_entry": dispatch_entry, } ) return records, warnings +def _dispatch_entry(manifest: dict) -> str | None: + config = manifest.get(OPT_OUT_KEY) + if not isinstance(config, dict): + return None + raw = config.get(DISPATCH_KEY) + if raw is True: + return DEFAULT_DISPATCH_ENTRY + if isinstance(raw, dict): + entry = raw.get("entry") + if isinstance(entry, str) and entry.strip(): + return entry.strip() + return None + + def _parse_command(command: str, harness: str) -> tuple[str, str, str] | None: match = DIRECT_RE.fullmatch(command) if not match or match.group(1) != harness: @@ -211,7 +236,22 @@ def _payload(stdin: bytes) -> dict[str, object]: return parsed if isinstance(parsed, dict) else {} -def _run_one(hooks_root: str, python: str, record: dict, stdin: bytes, budget: float) -> dict: +def _run_one( + hooks_root: str, + python: str, + record: dict, + stdin: bytes, + budget: float, + parsed_event: dict[str, object] | None = None, +) -> dict: + if record.get("dispatch_entry") and _entrypoint_exists(hooks_root, record["hook"], str(record["dispatch_entry"])): + return _run_sdk_one(hooks_root, record, stdin, budget, parsed_event) + return _run_subprocess_one(hooks_root, python, record, stdin, budget, fallback=True) + + +def _run_subprocess_one( + hooks_root: str, python: str, record: dict, stdin: bytes, budget: float, fallback: bool = False +) -> dict: hook, script = record["hook"], record["script"] started = time.monotonic() script_path = os.path.join(hooks_root, hook, script) @@ -249,6 +289,8 @@ def _run_one(hooks_root: str, python: str, record: dict, stdin: bytes, budget: f run._delete_findings_file(findings_path) result_outcome = outcome.classify(exit_code, stdout, stderr, timed_out) row = run._row(hooks_root, hook, script, stdin, result_outcome, exit_code, started, stdout, stderr, rule_ids) + if fallback: + row["dispatch_path"] = "subprocess_fallback" return { "hook": hook, "script": script, @@ -260,12 +302,158 @@ def _run_one(hooks_root: str, python: str, record: dict, stdin: bytes, budget: f } +def _sdk_dirs(hooks_root: str) -> tuple[str, str]: + engine_hooks = os.path.abspath(hooks_root) + sdk_dir = os.path.join(engine_hooks, "_sdk") + return engine_hooks, sdk_dir + + +def _prefer_import_path(path: str, index: int) -> None: + try: + sys.path.remove(path) + except ValueError: + pass + sys.path.insert(index, path) + + +def _load_entrypoint(hooks_root: str, hook: str, entry: str): + module_name, sep, function_name = entry.partition(":") + if not sep or not module_name or not function_name: + raise ValueError(f"dispatch entry must be MODULE:FUNCTION, got {entry!r}") + module_path = _entrypoint_path(hooks_root, hook, module_name) + if not os.path.isfile(module_path): + raise FileNotFoundError(module_path) + _engine_hooks, sdk_dir = _sdk_dirs(hooks_root) + _prefer_import_path(sdk_dir, 0) + _prefer_import_path(os.path.dirname(module_path), 1) + unique_name = f"_catstack_dispatch_{hook.replace('-', '_')}_{abs(hash(module_path))}" + spec = importlib.util.spec_from_file_location(unique_name, module_path) + if spec is None or spec.loader is None: + raise ImportError(f"could not load {module_path}") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + target = getattr(module, function_name) + if not callable(target): + raise TypeError(f"{entry!r} did not resolve to a callable") + return target + + +def _entrypoint_path(hooks_root: str, hook: str, module_name: str) -> str: + return os.path.join(hooks_root, hook, *module_name.split(".")) + ".py" + + +def _entrypoint_exists(hooks_root: str, hook: str, entry: str) -> bool: + module_name, sep, function_name = entry.partition(":") + return bool(sep and module_name and function_name and os.path.isfile(_entrypoint_path(hooks_root, hook, module_name))) + + +def _event(stdin: bytes, harness: str, event_name: str) -> dict[str, object]: + raw = stdin.decode("utf-8", errors="replace") + try: + parsed = json.loads(raw) + except json.JSONDecodeError as exc: + parsed = { + "_raw_payload": raw, + "_payload_error": f"the hook payload is not JSON ({exc})", + "_payload_error_type": type(exc).__name__, + "_payload_error_detail": str(exc), + } + if not isinstance(parsed, dict): + parsed = {"_raw_payload": raw, "_payload_error": "the hook payload is not a JSON object"} + parsed.setdefault("_raw_payload", raw) + parsed.setdefault("_catstack_harness", harness) + parsed.setdefault("hook_event_name", event_name) + return parsed + + +def _render_findings(hooks_root: str, hook: str, harness: str, event_name: str, event: dict[str, object], findings: list): + _engine_hooks, sdk_dir = _sdk_dirs(hooks_root) + _prefer_import_path(sdk_dir, 0) + from modes import effective_finding_modes + from render import render + from runtime import _renderable_findings + + mode, _mode_source, finding_modes = effective_finding_modes(hook, event, findings) + rendered_mode, rendered_findings = _renderable_findings(finding_modes, findings, mode) + stdout_text, stderr_text, exit_code = render(harness, event_name, rendered_mode, rendered_findings) + return stdout_text.encode(), stderr_text.encode(), exit_code, [finding.rule_id for finding in findings] + + +def _run_sdk_one( + hooks_root: str, + record: dict, + stdin: bytes, + budget: float, + parsed_event: dict[str, object] | None = None, +) -> dict: + hook, script = record["hook"], record["script"] + started = time.monotonic() + stdout = b"" + stderr = b"" + exit_code = 0 + timed_out = False + rule_ids: list[str] = [] + harness = run._harness(hooks_root) + payload = dict(parsed_event) if parsed_event is not None else _event(stdin, harness, "SubagentStop") + previous_handler = None + try: + detect = _load_entrypoint(hooks_root, hook, str(record["dispatch_entry"])) + if hasattr(signal, "SIGALRM"): + previous_handler = signal.getsignal(signal.SIGALRM) + + def _timeout(_signum, _frame): + raise TimeoutError(f"{hook}/{record['dispatch_entry']} timed out after {budget}s") + + signal.signal(signal.SIGALRM, _timeout) + signal.setitimer(signal.ITIMER_REAL, budget) + findings = detect(payload) + if not isinstance(findings, list): + raise TypeError("detect(event) must return a list") + stdout, stderr, exit_code, rule_ids = _render_findings(hooks_root, hook, harness, "SubagentStop", payload, findings) + lines = payload.get("_catstack_stderr_lines") + if isinstance(lines, list): + stderr += "".join(f"{line}\n" for line in lines if isinstance(line, str) and line).encode() + except TimeoutError: + timed_out = True + exit_code = 1 + stdout = b"" + stderr = f"catstack-hook-runner: {hook}/{script} timed out after {budget}s\n".encode() + except BaseException as exc: + if isinstance(exc, KeyboardInterrupt): + raise + if isinstance(exc, SystemExit): + exit_code = exc.code if isinstance(exc.code, int) else 1 + else: + exit_code = 0 + stdout = b"" + stderr = f"catstack-hook-error {hook}: {type(exc).__name__}: {exc}\n".encode() + finally: + if hasattr(signal, "SIGALRM"): + signal.setitimer(signal.ITIMER_REAL, 0) + if previous_handler is not None: + signal.signal(signal.SIGALRM, previous_handler) + result_outcome = outcome.classify(exit_code, stdout, stderr, timed_out) + row = run._row(hooks_root, hook, script, stdin, result_outcome, exit_code, started, stdout, stderr, rule_ids) + row["dispatch_path"] = "sdk" if not timed_out and not stderr.startswith(b"catstack-hook-error ") else "sdk_error" + return { + "hook": hook, + "script": script, + "outcome": result_outcome, + "exit_code": exit_code, + "stdout": stdout, + "stderr": stderr, + "row": row, + } + + def _merge_stdout(results: list[dict]) -> tuple[bytes, bytes, int]: blocked = [r for r in results if r["outcome"] == "blocked"] spoke = [r for r in results if r["stdout"].strip()] if blocked: return _merge_blocks(blocked) if spoke: + if len(spoke) == 1: + return spoke[0]["stdout"], b"", spoke[0]["exit_code"] contexts = [note for r in spoke for note in (_note(r),) if note] payload: dict[str, object] = {"continue": True} if contexts: @@ -340,11 +528,9 @@ def run_dispatch( ).encode() return 1, b"", stderr, [] - results: list[dict] = [] - with concurrent.futures.ThreadPoolExecutor(max_workers=len(records)) as pool: - futures = [pool.submit(_run_one, hooks_root, python, record, stdin, budget) for record in records] - for future in futures: - results.append(future.result()) + detector_budget = min(budget, DEFAULT_SUBAGENT_STOP_DETECTOR_BUDGET) if event == "SubagentStop" else budget + parsed_event = _event(stdin, harness, event) if event == "SubagentStop" else None + results = [_run_one(hooks_root, python, record, stdin, detector_budget, parsed_event) for record in records] stdout, block_notes, exit_code = _merge_stdout(results) stderr_lines = [r["stderr"] for r in results if r["stderr"]] @@ -355,7 +541,7 @@ def run_dispatch( def _parse_args(argv: list[str]) -> argparse.Namespace: parser = argparse.ArgumentParser() parser.add_argument("--event", required=True) - parser.add_argument("--timeout", type=float, default=DEFAULT_BUDGET) + parser.add_argument("--timeout", type=float) return parser.parse_args(argv) @@ -364,7 +550,10 @@ def main(argv: list[str] | None = None) -> int: stdin = sys.stdin.buffer.read() hooks_root = run._hooks_root() harness = run._harness(hooks_root) - exit_code, stdout, stderr, rows = run_dispatch(hooks_root, harness, args.event, stdin, args.timeout) + budget = args.timeout + if budget is None: + budget = DEFAULT_SUBAGENT_STOP_EVENT_BUDGET if args.event == "SubagentStop" else DEFAULT_BUDGET + exit_code, stdout, stderr, rows = run_dispatch(hooks_root, harness, args.event, stdin, budget) metrics_path = run._metrics_path() metrics_errors = b"".join(run._write_metrics(row, metrics_path) for row in rows) sys.stdout.buffer.write(stdout) diff --git a/engine/hooks/_runner/report.py b/engine/hooks/_runner/report.py index 7ea63f7ac..c04dedd3e 100644 --- a/engine/hooks/_runner/report.py +++ b/engine/hooks/_runner/report.py @@ -227,6 +227,12 @@ def format_rate(value: float | None) -> str: return f"{value:.2f}" +def format_decimal(value: float | None) -> str: + if value is None: + return "-" + return f"{value:.1f}" + + def first_line(value: object) -> str: if not isinstance(value, str): return "" @@ -722,9 +728,111 @@ def format_harness_gap_table(report: dict[str, Any]) -> str: return "\n".join(lines) + "\n" +def _scorecard_phase(row: dict[str, Any]) -> str: + dispatch_path = row.get("dispatch_path") + return "after" if isinstance(dispatch_path, str) and dispatch_path else "before" + + +def _duration_ms(row: dict[str, Any]) -> int | None: + duration = row.get("duration_ms") + if isinstance(duration, int) and not isinstance(duration, bool): + return duration + return None + + +def _event_group_key(row: dict[str, Any], index: int) -> tuple[str, str, str, str]: + phase = _scorecard_phase(row) + event = str(row.get("event") or "(unknown)") + session_id = str(row.get("session_id") or "") + event_uid = row.get("event_uid") + uid = str(event_uid) if isinstance(event_uid, str) and event_uid else f"row-{index}" + return phase, event, session_id, uid + + +def _group_wall_ms(rows: list[dict[str, Any]]) -> int: + starts = [] + ends = [] + fallback_total = 0 + for row in rows: + duration = _duration_ms(row) or 0 + fallback_total += duration + end = parse_ts(row.get("ts")) + if end is None: + continue + starts.append(end - timedelta(milliseconds=duration)) + ends.append(end) + if starts and ends: + return max(0, int((max(ends) - min(starts)).total_seconds() * 1000)) + return fallback_total + + +def _group_procs(rows: list[dict[str, Any]], phase: str) -> int: + if phase == "after": + fallback = sum(1 for row in rows if row.get("dispatch_path") == "subprocess_fallback") + return 1 + fallback + return len(rows) + + +def build_scorecard(rows: list[dict[str, Any]], malformed: int, warnings: list[str]) -> dict[str, Any]: + groups: dict[tuple[str, str, str, str], list[dict[str, Any]]] = {} + for index, row in enumerate(rows): + groups.setdefault(_event_group_key(row, index), []).append(row) + + event_groups: dict[tuple[str, str], list[list[dict[str, Any]]]] = {} + for (phase, event, _session_id, _uid), group_rows in groups.items(): + event_groups.setdefault((phase, event), []).append(group_rows) + + scorecard_rows = [] + for (phase, event), grouped_rows in sorted(event_groups.items()): + flat = [row for group_rows in grouped_rows for row in group_rows] + detector_runs = len(flat) + events = len(grouped_rows) + procs = sum(_group_procs(group_rows, phase) for group_rows in grouped_rows) + cpu_ms = sum(_duration_ms(row) or 0 for row in flat) + wall_ms = sum(_group_wall_ms(group_rows) for group_rows in grouped_rows) + timeouts = sum(1 for row in flat if row.get("outcome") == "timed_out") + crashes = sum(1 for row in flat if row.get("outcome") in {"crashed", "caught_error"}) + spoke = sum(1 for row in flat if row.get("outcome") == "spoke") + scorecard_rows.append( + { + "phase": phase, + "event": event, + "events": events, + "detector_runs": detector_runs, + "procs_per_event": procs / events if events else None, + "cpu_ms_per_event": cpu_ms / events if events else None, + "wall_ms_per_event": wall_ms / events if events else None, + "timeout_rate": timeouts / detector_runs if detector_runs else None, + "crash_rate": crashes / detector_runs if detector_runs else None, + "spoke_rate": spoke / detector_runs if detector_runs else None, + } + ) + return {"malformed_rows": malformed, "warnings": warnings, "scorecard": scorecard_rows} + + +def format_scorecard_table(report: dict[str, Any]) -> str: + lines = list(report["warnings"]) + if report["malformed_rows"]: + lines.append(f"skipped {report['malformed_rows']} malformed row(s)") + lines.append( + "phase event events detector_runs procs/event cpu_ms/event wall_ms/event timeout% crash% spoke-rate" + ) + for row in report["scorecard"]: + timeout_pct = row["timeout_rate"] * 100 if row["timeout_rate"] is not None else None + crash_pct = row["crash_rate"] * 100 if row["crash_rate"] is not None else None + lines.append( + f"{row['phase']} {row['event']} {row['events']} {row['detector_runs']} " + f"{format_decimal(row['procs_per_event'])} {format_decimal(row['cpu_ms_per_event'])} " + f"{format_decimal(row['wall_ms_per_event'])} {format_decimal(timeout_pct)} " + f"{format_decimal(crash_pct)} {format_rate(row['spoke_rate'])}" + ) + return "\n".join(lines) + "\n" + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser() parser.add_argument("--since", default="7d") + parser.add_argument("--days", type=float) parser.add_argument("--json", action="store_true") mode = parser.add_mutually_exclusive_group() mode.add_argument("--events", action="store_true", default=True) @@ -732,15 +840,30 @@ def main(argv: list[str] | None = None) -> int: mode.add_argument("--judge", action="store_true") mode.add_argument("--skills", action="store_true") mode.add_argument("--harness-gap", action="store_true") + mode.add_argument("--scorecard", action="store_true") parser.add_argument("--grace", default="1h") parser.add_argument("--check", action="store_true") args = parser.parse_args(argv) try: - since = parse_since(args.since) + since = timedelta(days=args.days) if args.days is not None else parse_since(args.since) grace = parse_since(args.grace) except ValueError as exc: print(str(exc), file=sys.stderr) return 2 + if args.days is not None and args.days <= 0: + print("--days must be positive", file=sys.stderr) + return 2 + if args.scorecard: + rows, malformed, error = read_rows(metrics_path(), datetime.now(timezone.utc) - since) + if error is not None: + print(error) + return 2 + report = build_scorecard(rows or [], malformed, []) + if args.json: + print(json.dumps(report, sort_keys=True)) + else: + print(format_scorecard_table(report), end="") + return 0 if args.harness_gap: threshold = datetime.now(timezone.utc) - since home = Path(os.path.expanduser("~")) diff --git a/engine/hooks/_runner/run.py b/engine/hooks/_runner/run.py index 081747e3c..42fa17fc5 100644 --- a/engine/hooks/_runner/run.py +++ b/engine/hooks/_runner/run.py @@ -2,6 +2,7 @@ import argparse import datetime +import hashlib import json import os import subprocess @@ -234,6 +235,7 @@ def _row( "hook": hook, "script": script, "event": event, + "event_uid": hashlib.sha256(stdin).hexdigest()[:12], "session_id": session_id, "outcome": outcome, "exit_code": exit_code, diff --git a/engine/hooks/_runner/tests/test_dispatch.py b/engine/hooks/_runner/tests/test_dispatch.py index 05bd2a7c5..20dc6e02c 100644 --- a/engine/hooks/_runner/tests/test_dispatch.py +++ b/engine/hooks/_runner/tests/test_dispatch.py @@ -7,6 +7,7 @@ import sys import tempfile import unittest +from unittest import mock RUNNER_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -23,15 +24,34 @@ def setUp(self): for name in ("run.py", "outcome.py", "dispatch.py"): shutil.copy2(os.path.join(RUNNER_DIR, name), os.path.join(self.runner_dir, name)) - def _write_hook(self, name: str, script: str, body: str, event: str = "PreToolUse", matcher=None) -> None: + def _install_sdk(self) -> None: + hooks_dir = os.path.dirname(RUNNER_DIR) + shutil.copytree(os.path.join(hooks_dir, "_sdk"), os.path.join(self.hooks_root, "_sdk")) + shutil.copytree(os.path.join(hooks_dir, "_flags"), os.path.join(self.hooks_root, "_flags")) + + def _write_hook( + self, + name: str, + script: str, + body: str, + event: str = "PreToolUse", + matcher=None, + subagent_stop: dict[str, object] | None = None, + extra_files: dict[str, str] | None = None, + ) -> None: hook_dir = os.path.join(self.hooks_root, name) os.makedirs(hook_dir) with open(os.path.join(hook_dir, script), "w", encoding="utf-8") as handle: handle.write(body) + for filename, contents in (extra_files or {}).items(): + with open(os.path.join(hook_dir, filename), "w", encoding="utf-8") as handle: + handle.write(contents) entry = {"hooks": [{"type": "command", "command": f"python3 $HOME/.claude/hooks/{name}/{script}"}]} if matcher is not None: entry["matcher"] = matcher manifest = {"hooks": {event: [entry]}} + if subagent_stop is not None: + manifest["subagent_stop"] = subagent_stop with open(os.path.join(hook_dir, "claude.hook.json"), "w", encoding="utf-8") as handle: json.dump(manifest, handle) @@ -173,6 +193,74 @@ def test_matcher_on_the_manifest_filters_which_hooks_run(self): self.assertEqual(rows_outcome(self._rows(), "fixture-bash"), "spoke") self.assertEqual(self._rows(), [row for row in self._rows() if row["hook"] == "fixture-bash"]) + def test_subagent_stop_dispatch_only_loads_manifest_opted_in_hooks(self): + self._write_hook( + "fixture-dispatched", + "ok.py", + "import json\nprint(json.dumps({'hookSpecificOutput': {'additionalContext': 'dispatched'}}))\n", + event="Stop", + subagent_stop={"dispatch": True}, + ) + self._write_hook( + "fixture-direct", + "direct.py", + "import json\nprint(json.dumps({'hookSpecificOutput': {'additionalContext': 'direct'}}))\n", + event="Stop", + ) + + result = self._run(event="SubagentStop") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(b"dispatched", result.stdout) + self.assertNotIn(b"direct", result.stdout) + rows = self._rows() + self.assertEqual([row["hook"] for row in rows], ["fixture-dispatched"]) + + def test_missing_sdk_entrypoint_falls_back_to_the_existing_script_and_marks_the_row(self): + self._write_hook( + "fixture-fallback", + "ok.py", + "import json\nprint(json.dumps({'hookSpecificOutput': {'additionalContext': 'fallback ran'}}))\n", + event="Stop", + subagent_stop={"dispatch": True}, + ) + + result = self._run(event="SubagentStop") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(b"fallback ran", result.stdout) + row = self._row_for("fixture-fallback") + self.assertEqual(row["dispatch_path"], "subprocess_fallback") + + def test_sdk_entrypoint_runs_in_process_and_emits_rule_ids(self): + self._install_sdk() + self._write_hook( + "fixture-sdk", + "ok.py", + "print('subprocess should not run')\n", + event="Stop", + subagent_stop={"dispatch": {"entry": "detect:detect"}}, + extra_files={ + "detect.py": ( + "from finding import Finding\n" + "def detect(event):\n" + " return [Finding(rule_id='fixture.sdk', subject='s', message='sdk spoke', evidence='e')]\n" + ) + }, + ) + env_patch = mock.patch.dict(os.environ, {"CATSTACK_HOOK_MODE_FIXTURE_SDK": "warn"}) + env_patch.start() + self.addCleanup(env_patch.stop) + + result = self._run(event="SubagentStop") + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(b"sdk spoke", result.stdout) + self.assertNotIn(b"subprocess should not run", result.stdout) + row = self._row_for("fixture-sdk") + self.assertEqual(row["dispatch_path"], "sdk") + self.assertEqual(row["rule_ids"], ["fixture.sdk"]) + FIXTURES_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "fixtures") FIXTURE_HOOK = "fixture-extra-manifest" @@ -283,12 +371,7 @@ def test_a_speaking_hook_in_an_extra_manifest_reaches_dispatch_with_the_run_alon self.assertEqual(alone.stderr, expected_stderr) self.assertEqual(dispatched.returncode, 0, dispatched.stderr) - self.assertEqual( - dispatched.stdout, - json.dumps( - {"continue": True, "additionalContext": f"[{FIXTURE_HOOK}] {FIXTURE_MESSAGE}"} - ).encode(), - ) + self.assertEqual(dispatched.stdout, alone.stdout) self.assertEqual(dispatched.stderr, expected_stderr) alone_row = self._only_row(alone_rows) @@ -421,12 +504,7 @@ def test_a_speaking_installed_hook_reaches_dispatch_with_the_run_alone_contract( self.assertEqual(alone.stderr, expected_stderr) self.assertEqual(dispatched.returncode, 0, dispatched.stderr) - self.assertEqual( - dispatched.stdout, - json.dumps( - {"continue": True, "additionalContext": f"[{INSTALLED_HOOK}] {FIXTURE_MESSAGE}"} - ).encode(), - ) + self.assertEqual(dispatched.stdout, alone.stdout) self.assertEqual(dispatched.stderr, expected_stderr) alone_row = self._only_row(alone_rows) @@ -480,7 +558,7 @@ def test_rewrapping_an_already_collapsed_cursor_install_keeps_its_hooks(self): env=env, ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn(f"[{INSTALLED_HOOK}] {FIXTURE_MESSAGE}".encode(), result.stdout) + self.assertIn(FIXTURE_MESSAGE.encode(), result.stdout) def test_an_unreadable_cursor_registry_is_reported_not_read_as_no_hooks(self): home = self._install("cursor", "speak", "1") diff --git a/engine/hooks/_runner/tests/test_report.py b/engine/hooks/_runner/tests/test_report.py index 909ab3731..e5c4609fc 100644 --- a/engine/hooks/_runner/tests/test_report.py +++ b/engine/hooks/_runner/tests/test_report.py @@ -382,6 +382,88 @@ def test_runs_report_counts_rows_across_rotated_files(self) -> None: self.assertEqual(result.returncode, 0, result.stderr) self.assertIn("claude hook-a/a.py 3 1 0 0 1 0 1 10", result.stdout) + def test_scorecard_compares_before_and_after_dispatch_path_rows(self) -> None: + self.write_rows( + [ + self.row( + "claude", + "hook-a", + "a.py", + "spoke", + event="SubagentStop", + event_uid="pre", + duration_ms=10, + ), + self.row( + "claude", + "hook-b", + "b.py", + "timed_out", + event="SubagentStop", + event_uid="pre", + duration_ms=20, + ), + self.row( + "claude", + "hook-a", + "a.py", + "spoke", + event="SubagentStop", + event_uid="post", + duration_ms=5, + dispatch_path="sdk", + ), + self.row( + "claude", + "hook-b", + "b.py", + "crashed", + event="SubagentStop", + event_uid="post", + duration_ms=7, + dispatch_path="subprocess_fallback", + ), + ] + ) + + result = self.run_report("--scorecard", "--days", "14", "--json") + + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + rows = {(row["phase"], row["event"]): row for row in json.loads(result.stdout)["scorecard"]} + self.assertEqual(rows[("before", "SubagentStop")]["events"], 1) + self.assertEqual(rows[("before", "SubagentStop")]["detector_runs"], 2) + self.assertEqual(rows[("before", "SubagentStop")]["procs_per_event"], 2) + self.assertEqual(rows[("before", "SubagentStop")]["cpu_ms_per_event"], 30) + self.assertEqual(rows[("before", "SubagentStop")]["timeout_rate"], 0.5) + self.assertEqual(rows[("after", "SubagentStop")]["events"], 1) + self.assertEqual(rows[("after", "SubagentStop")]["detector_runs"], 2) + self.assertEqual(rows[("after", "SubagentStop")]["procs_per_event"], 2) + self.assertEqual(rows[("after", "SubagentStop")]["cpu_ms_per_event"], 12) + self.assertEqual(rows[("after", "SubagentStop")]["crash_rate"], 0.5) + + def test_scorecard_prints_dashboard_columns(self) -> None: + self.write_rows( + [ + self.row("claude", "hook-a", "a.py", "spoke", event="SubagentStop", event_uid="pre"), + self.row( + "claude", + "hook-a", + "a.py", + "spoke", + event="SubagentStop", + event_uid="post", + dispatch_path="sdk", + ), + ] + ) + + result = self.run_report("--scorecard", "--days", "14") + + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("phase event events detector_runs procs/event cpu_ms/event wall_ms/event timeout% crash% spoke-rate", result.stdout) + self.assertIn("before SubagentStop 1 1 1.0 10.0", result.stdout) + self.assertIn("after SubagentStop 1 1 1.0 10.0", result.stdout) + def test_event_report_suggests_each_mode_change(self) -> None: rows: list[dict[str, object]] = [] rows += self.closed_events( diff --git a/engine/hooks/_runner/tests/test_wrap_installed.py b/engine/hooks/_runner/tests/test_wrap_installed.py index 008b06bd4..6de0ec8a3 100644 --- a/engine/hooks/_runner/tests/test_wrap_installed.py +++ b/engine/hooks/_runner/tests/test_wrap_installed.py @@ -24,6 +24,9 @@ def setUp(self): self.old_home = os.environ.get("HOME") os.environ["HOME"] = str(self.home) self.addCleanup(self._restore_home) + dispatcher_patch = mock.patch.dict(os.environ, {"CATSTACK_HOOK_DISPATCHER": "0"}) + dispatcher_patch.start() + self.addCleanup(dispatcher_patch.stop) self.claude_path = self.home / ".claude" / "settings.json" self.cursor_path = self.home / ".cursor" / "hooks.json" self.codex_path = self.home / ".codex" / "hooks.json" diff --git a/engine/hooks/llm-judge/phrases/restated-rule.json b/engine/hooks/llm-judge/phrases/restated-rule.json index 4d758763b..0f0acdbe1 100644 --- a/engine/hooks/llm-judge/phrases/restated-rule.json +++ b/engine/hooks/llm-judge/phrases/restated-rule.json @@ -19,5 +19,5 @@ "we don't have a rule for this yet, can you add one?", "1. yes 2. hold 3. all 4. yes. 5. how much tokens is this all costing me so far?" ], - "on_hit": "restated-rule: the person had to restate a rule or skill that already exists. Count it as an intervention; a restated rule the agent should already follow is automate-me evidence." + "on_hit": "restated-rule: the person had to restate a rule or skill that already exists. Count it as an intervention signal that orders the reflect pass. Default lesson bucket is already-covered (execution miss on an existing skill) via engine/skills/reflect/scripts/lesson_route.py — do not invent a new preference or call automate-me for this hit alone." } diff --git a/engine/hooks/no-comments/detect.py b/engine/hooks/no-comments/detect.py index 17f73ff42..fc4ee4e28 100644 --- a/engine/hooks/no-comments/detect.py +++ b/engine/hooks/no-comments/detect.py @@ -53,7 +53,7 @@ TRAILING_SLASH_RE = re.compile(r"\s//\s*\S") TRAILING_DIRECTIVE_RE = re.compile( r"\s(?:#|//)\s*(?:noqa|type:|pragma|pylint|mypy|ruff|fmt:|nosec|isort|nolint|" - r"eslint|prettier|@ts-|tslint|istanbul|biome|swiftlint|NOSONAR)", + r"eslint|prettier|@ts-|tslint|istanbul|biome|swiftlint|NOSONAR|text-match-decision-warn:)", re.IGNORECASE, ) RULE_ADDED_COMMENT = "no-comments.added-comment" diff --git a/engine/hooks/no-comments/tests/test_hooks.py b/engine/hooks/no-comments/tests/test_hooks.py index 211e0c528..0de7181dc 100644 --- a/engine/hooks/no-comments/tests/test_hooks.py +++ b/engine/hooks/no-comments/tests/test_hooks.py @@ -133,7 +133,7 @@ def test_blocks_write_and_multiedit_shapes(self): class TestAllows(unittest.TestCase): def test_allows_directives_and_shebang(self): - text = "#!/usr/bin/env python3\n# -*- coding: utf-8 -*-\nimport x # noqa: E402\ny = 1 # type: ignore\n# pragma: no cover\n" + text = "#!/usr/bin/env python3\n# -*- coding: utf-8 -*-\nimport x # noqa: E402\ny = 1 # type: ignore\n# pragma: no cover\nz = pat.search(text) # text-match-decision-warn: allow\n" self.assertEqual(detect.comment_lines("/r/a.py", text), []) js = "// eslint-disable-next-line no-console\n// @ts-ignore\n/* istanbul ignore next */\n// SPDX-License-Identifier: MIT\n" self.assertEqual(detect.comment_lines("/r/a.ts", js), []) diff --git a/engine/skills/reflect/SKILL.md b/engine/skills/reflect/SKILL.md index a7bbf10fd..834327b6c 100644 --- a/engine/skills/reflect/SKILL.md +++ b/engine/skills/reflect/SKILL.md @@ -1,6 +1,6 @@ --- name: reflect -description: Mine a conversation transcript — and the commit history of the files it touched — for durable learnings, then route Accepted items into concrete skill/code edits via an auto-spawned worktree in the repo that owns the named skill + PR (never merge). Working-style preferences route to the sibling skill automate-me, not a task skill edit. User involvement, forced restatement, "you fucked up/messed up", or the same type of complaint twice is a FAILURE and must route to automate-me. Use when the user says reflect, after a complex multi-step task lands cleanly and the recipe is worth keeping, when the agent hit dead ends before finding a working path, or when the user corrected the agent's approach mid-task. +description: Mine a conversation transcript — and the commit history of the files it touched — for durable learnings, then route Accepted items into concrete skill/code edits via an auto-spawned worktree in the repo that owns the named skill + PR (never merge). Working-style preferences route to the sibling skill automate-me, not a task skill edit. User involvement, forced restatement, "you fucked up/messed up", or the same type of complaint twice is a FAILURE that orders the pass; lesson_route.py decides the bucket — a restated existing rule is already-covered, not a new preference. Use when the user says reflect, after a complex multi-step task lands cleanly and the recipe is worth keeping, when the agent hit dead ends before finding a working path, or when the user corrected the agent's approach mid-task. disable-model-invocation: true --- @@ -60,7 +60,7 @@ whole context. - The user said "you fucked up", "you messed up", "I told you", "you're ignoring me", or equivalent agent-blame. Treat this reflect pass as FAIL. The class is *ignored named constraint*, not the swear word. Product-blame ("the UI is messed up") is not this class. - The reply conceded the user's suspicion — "your instinct was right", "your hunch was right", "good catch" followed by a correction. Conceding means the user found what the agent's own checks did not, so the miss is the finding, not the concession. Treat as FAIL; the `wrong-check-reflect` detector fires on these shapes. - The user had to keep iterating, restate requirements, or change product direction because the agent missed something already named. FAIL, then `automate-me`. A genuine mind-change (user learned new facts, then redirected) is not failure. A forced restatement of an already-named constraint is. -- The same *type* of complaint appears in 2+ turns or 2+ sessions (repro-then-fix, UI proof before done, e2e before claiming pass, obey the named verb). That class is a bug. **Must** invoke `automate-me` — not optional, do not wait for the user to say "automate me." `token_audit.py`'s `intervention-must-automate` flag is the mechanical catch; human-message only, never tool_result / skill-injection / `/loop` polls. Run it with `--judge` so a restated existing rule is judged; without it the flag reads `unchecked`, never a clean `no`. +- The same *type* of complaint appears in 2+ turns or 2+ sessions (repro-then-fix, UI proof before done, e2e before claiming pass, obey the named verb). That class is a bug. Run `lesson_route.py` before treating it as a new preference: a restated existing rule is `already-covered` (execution miss), not `automate-me`. Only a working preference that no skill already states routes to `automate-me` — not optional once that bucket wins, do not wait for the user to say "automate me." `token_audit.py`'s `intervention-must-automate` flag is the mechanical catch that *orders* the pass; human-message only, never tool_result / skill-injection / `/loop` polls. Run it with `--judge` so a restated existing rule is judged; without it the flag reads `unchecked`, never a clean `no`. The flag does not by itself create an automate-me row. - It's been a while since the corpus-wide pass (`top_sessions.py` + this skill's lenses across the worst offenders) last ran. No fixed cadence and no cron — just periodically worth doing by hand. - The user asks *why does X keep happening* across a span of time or across machines — that's **multi-conversation mode**; read [references/corpus-scan.md](references/corpus-scan.md). - The invocation is itself an automated `reflect-ci-*` task with no human in the loop: run the step-3 sibling check unconditionally — concurrent automated dispatches produce exactly the duplicate-work burst a human would otherwise be there to notice. @@ -116,13 +116,44 @@ When the budget gate reduced step 3, synthesis must include the exact reduced status line above before findings. It must name every omitted lens, even when all required lenses returned. -- **Accepted** — real, durable, worth acting on. Apply the elimination hierarchy from step 3 before slotting a finding here as a skill edit: if a reviewer proposed a skill/rule fix but a categorical or lint/test fix was actually available, bump it to Backlog with the stronger fix named instead, or split it. +**Lesson-route gate (runs before any Accepted or automate-me row).** For each +candidate finding, run +`python3 engine/skills/reflect/scripts/lesson_route.py` with the incident +quote, the assistant reply, the target skill text (if any), whether the +transcript opened that skill, and the kind (`restated-rule` / +`preference` / durable lesson). Its stdout `bucket` wins over the model's +sort. Buckets: + +- Restated existing rule, or an admitted skip of a rule that already + exists: Rejected. Do not open preference capture. "No known prior art" + does not override. (bucket `already-covered`, for example after + "It is; I skipped it.") +- Named skill never opened in the transcript: Rejected, or tune its + description when the miss was a trigger miss. (bucket `skill-not-used`) +- When the person states how they like future sessions to behave and no + skill already says that: hand it to the preference-capture sibling skill + rather than editing a task skill. (bucket `automate-me`) +- Durable and decision-changing new lesson: Accepted. Frustration and the + intervention flag order the pass; it does not by itself create an automate-me row or select Accepted. (bucket `accepted`) + +- **Accepted** — real, durable, worth acting on, and `lesson_route.py` + returned `accepted`. Apply the elimination hierarchy from step 3 before + slotting a finding here as a skill edit: if a reviewer proposed a + skill/rule fix but a categorical or lint/test fix was actually available, + bump it to Backlog with the stronger fix named instead, or split it. - **Backlog** — real, but the right fix is higher up the hierarchy than a skill edit. Note which tier (1: categorical, 2: lint/test, 3: hook) each backlog item is. -- **Grounding gate for skill prose.** Before an Accepted item becomes skill prose, name the established principle it instantiates — author, title, year, and a checkable URL — or write "no known prior art". An incident-shaped rule with neither goes to Backlog for grounding, not to Accepted. A rule that restates one session's bug in fresh words reads as invented and drifts into an incident log; the field's own name for it (fail fast, invariant, completeness check, reconciliation) is what the skill should say. -- **Rejected** — one-offs, already covered, or too speculative. A rejection +- **Grounding gate for skill prose.** Before an Accepted item becomes skill prose, name the established principle it instantiates — author, title, year, and a checkable URL — or write "no known prior art". An incident-shaped rule with neither goes to Backlog for grounding, not to Accepted. A rule that restates one session's bug in fresh words reads as invented and drifts into an incident log; the field's own name for it (fail fast, invariant, completeness check, reconciliation) is what the skill should say. Grounding never overrides an `already-covered` lesson-route bucket. +- **Rejected** — one-offs, already covered (`lesson_route.py` bucket + `already-covered` or `skill-not-used`), or too speculative. A rejection that says already covered by X names X at file and line; a sentence a hook challenged gets evidence or a tag, never deletion. -- **Route to `automate-me`** — real, but it's about how *this user* likes to work rather than a lesson about the code or task. Don't inline these as edits to a task-specific skill; hand the finding to `automate-me`. Same-type complaints (2+ turns or 2+ sessions) and forced iteration / product-direction change after an agent miss are **mandatory** here, not optional. Invoke `automate-me` in the same turn if the user already asked to capture the preference, or name it as the first follow-up with evidence; do not wait for them to re-prompt. +- **Route to `automate-me`** — only when `lesson_route.py` returned + `automate-me`: a working preference that no existing skill states. Don't + inline these as edits to a task-specific skill; hand the finding to + `automate-me`. A restated existing rule is **not** this bucket. Invoke + `automate-me` in the same turn when that bucket wins and the user already + asked to capture the preference, or name it as the first follow-up with + evidence; do not wait for them to re-prompt. **Three required parts per finding.** Every Accepted, Backlog, and Route-to-`automate-me` finding states all three. A finding missing one is not ready to sort. @@ -144,13 +175,13 @@ Present the full Accepted / Backlog / Route-to-automate-me / Rejected list to th 3. That agent commits, opens a PR via `draft-pr` / `make-pr` (title prefix optional `[auto]` when unattended) **in the owning repo**, and **never merges** — external apply is never-merge in the owning checkout only, same as catstack. 4. Tell the user each worktree's path + branch + owning repo + that a PR is opening; they can veto in chat (“drop item 2”, “abort reflect apply”) — if the PR is not open yet, stop the agent and remove the worktree; if it is open, close/leave the PR per their instruction. -Chat approval is no longer the gate that *starts* Accepted application — the PR is. Backlog wait is for process, agents, and workers (durable automation) — not already-named execution, which dispatches immediately: Invoker if MCP/CLI is installed unless the user said not to, else a separate git worktree + PR stack. Do not auto-apply every Backlog row. Same-type / `intervention-must-automate` still invokes `automate-me` in the same turn; other Route-to-automate-me items still wait for an explicit yes. Never silently edit skills in the parent worktree while the reflect-apply worktree is the intended home for those edits. +Chat approval is no longer the gate that *starts* Accepted application — the PR is. Backlog wait is for process, agents, and workers (durable automation) — not already-named execution, which dispatches immediately: Invoker if MCP/CLI is installed unless the user said not to, else a separate git worktree + PR stack. Do not auto-apply every Backlog row. When `lesson_route.py` returned `automate-me`, invoke `automate-me` in the same turn; other Route-to-automate-me items still wait for an explicit yes. An `intervention-must-automate: yes` flag without that bucket only orders the investigation. Never silently edit skills in the parent worktree while the reflect-apply worktree is the intended home for those edits. If the session is an open product incident and synthesis already names a concrete product change, that change is the first Accepted item the worktree implements. Process hooks stay parallel backlog — do not offer only the hook or a proof plan when the named one-liner is what stops the live defect. Skip the auto-worktree only when the user said reflect-only / “don’t apply” / Accepted is empty / every Accepted item is already landed on an open PR. -If `token_audit.py` flagged `intervention-must-automate: yes`, or synthesis found the same complaint type twice, the first offered action is invoking `automate-me` (alongside any product one-liner). That is not a style note. +If `token_audit.py` flagged `intervention-must-automate: yes`, or synthesis found the same complaint type twice, run `lesson_route.py` on each candidate first. Present Rejected `already-covered` rows for restated existing rules; invoke `automate-me` only for rows whose bucket is `automate-me` (alongside any product one-liner). The flag orders the pass; it does not by itself create an automate-me row. ### 5b. Headless / session-mine mode diff --git a/engine/skills/reflect/references/cost-audit.md b/engine/skills/reflect/references/cost-audit.md index 8af99ec29..58e63892c 100644 --- a/engine/skills/reflect/references/cost-audit.md +++ b/engine/skills/reflect/references/cost-audit.md @@ -46,9 +46,9 @@ They also emit `brevity-hook-blocks`: how many replies the brevity checker itsel They also emit `brevity-follow-ups`: how many times the user had to ask for a shorter reply, counting `/diu` invocations (read from the command-name field, not matched out of prose) plus messages whose whole text is `eli5` / `eli 5`. This is the feed for judging whether the always-on brevity rule is working: a session where the user asked twice is a session where the default reply was too long twice. Codex, OMP, and Cursor transcripts carry no slash-command field, so those modes count the `eli5`-only messages and report `unchecked` when that count is zero — a zero there is not a clean count. `/diu` mentioned inside a longer sentence is not a request; only the typed command is. -They also emit `intervention-must-automate`: yes when a verbatim re-send fired, any intervention kind (`told-you`, `accusation`, `agent-blame`) appears ≥2 times, or ≥2 distinct intervention kinds appear in the session. One "I told you" is frustration only; the same class twice is FAIL and must route to `automate-me`. `/loop` polls and Stop-hook injection text are not the human complaining. +They also emit `intervention-must-automate`: yes when a verbatim re-send fired, any intervention kind (`told-you`, `accusation`, `agent-blame`) appears ≥2 times, or ≥2 distinct intervention kinds appear in the session. One "I told you" is frustration only; the same class twice is FAIL and *orders* the reflect pass. Run `lesson_route.py` before offering a lesson: a restated existing rule is `already-covered`, not a new preference. Only a `lesson_route.py` bucket of `automate-me` may invoke `automate-me`. `/loop` polls and Stop-hook injection text are not the human complaining. -Two more kinds count, both read from human messages only. `restated-after-rejection` is the person's next message after a tool call they rejected; the rejection comes from the harness's typed denial fields (`toolDenialKind: user-rejected`, `toolUseResult: User rejected tool use`), never from tool_result prose. `restated-rule` is a message that restates a rule, skill, or standing decision that already exists; its meaning is decided by the llm-judge against the `engine/hooks/llm-judge/phrases/restated-rule.json` dictionary, and only under `--judge`. Grow that dictionary from real misses (`phrase-judge`), never a regex. Without `--judge`, or when no judge runner answers, `intervention-must-automate` reports `unchecked` unless other evidence already makes it `yes`. +Two more kinds count, both read from human messages only. `restated-after-rejection` is the person's next message after a tool call they rejected; the rejection comes from the harness's typed denial fields (`toolDenialKind: user-rejected`, `toolUseResult: User rejected tool use`), never from tool_result prose. `restated-rule` is a message that restates a rule, skill, or standing decision that already exists; its meaning is decided by the llm-judge against the `engine/hooks/llm-judge/phrases/restated-rule.json` dictionary, and only under `--judge`. Grow that dictionary from real misses (`phrase-judge`), never a regex. Without `--judge`, or when no judge runner answers, `intervention-must-automate` reports `unchecked` unless other evidence already makes it `yes`. The phrase file's `on_hit` names `already-covered` as the default lesson bucket for that signal. To re-run the reality check after tuning the detector, use the shared runner `scripts/test/backtest_detector.py` from the repo root: diff --git a/engine/skills/reflect/references/lenses.md b/engine/skills/reflect/references/lenses.md index aa99bfc89..1b4d114a4 100644 --- a/engine/skills/reflect/references/lenses.md +++ b/engine/skills/reflect/references/lenses.md @@ -25,7 +25,7 @@ One message, parallel `Agent` calls (`subagent_type: general-purpose`), each giv | Cost | Given the `token_audit.py` output (not the raw transcript): which flagged items were genuinely avoidable thrash vs. required work (e.g. a Read immediately followed by an Edit on the same file is required by tool semantics, not thrash — a Read repeated with nothing changed in between is). Recommends concrete fixes: delegate a flagged lookup-only turn to a cheaper model or a fork, batch a run of small Edits into fewer passes, investigate a cache-creation spike, stop re-reading a file that didn't change. | | History | For the files this session touched (especially ones it debugged, reworked, or was corrected on), run `git log --follow -p -- ` and `git blame` on the changed lines *before* judging the session in isolation. Check whether an earlier commit — particularly one tagged `Co-Authored-By: Claude` — already introduced this exact bug, papered over the same symptom, or reworked this same area before. A session that "fixed" something we broke ourselves, or that re-solved a problem a past session already solved, is a stronger and more accountable finding than anything visible in the transcript alone — it means a skill or a fix didn't actually stick. | | Divergent | Whatever the other lenses would miss — an unconventional angle, a blind spot, a pattern that only shows up zoomed out. | -| Frustration | Every moment the user's tone spiked: all-caps runs, profanity, verbatim-repeated messages, "I told you", "I am waiting", explicit time-constraint mentions, rapid interruption/steering bursts, agent-blame ("you fucked up/messed up"). Enumerate each with its timestamp/index, trace it back to the specific agent decision(s) that provoked it, and rank the session's root causes primarily by frustration caused — tokens burned is the tiebreaker, not the metric. Feed this lens the mechanical frustration-signal output from `token_audit.py` (see cost-audit), never the raw JSONL. If `intervention-must-automate` is yes, this session is FAIL: the first offered action is `automate-me` (same-type complaint / forced restatement). Do not treat that flag as a preference ping. | +| Frustration | Every moment the user's tone spiked: all-caps runs, profanity, verbatim-repeated messages, "I told you", "I am waiting", explicit time-constraint mentions, rapid interruption/steering bursts, agent-blame ("you fucked up/messed up"). Enumerate each with its timestamp/index, trace it back to the specific agent decision(s) that provoked it, and rank the session's root causes primarily by frustration caused — tokens burned is the tiebreaker, not the metric. Feed this lens the mechanical frustration-signal output from `token_audit.py` (see cost-audit), never the raw JSONL. If `intervention-must-automate` is yes, this session is FAIL and the flag *orders* the pass: run `lesson_route.py` before offering a lesson. A restated existing rule is `already-covered` (execution miss), not a new preference. Only a `lesson_route.py` bucket of `automate-me` may invoke `automate-me`. Frustration does not select the Accepted bucket. | Each reviewer returns candidate learnings as: what happened (with a quote/reference), why it matters, a suggested routing, and — for anything proposed as skill prose — the established principle it instantiates (author, title, year, URL) or the words "no known prior art", so the synthesis step's grounding gate has something to check. For any finding that traces back to a point where the user (or a past self) had to intervene and correct the agent mid-task, the reviewer must first ask *how would this stop needing a correction at all* — not just "what skill line would have prevented it." The synthesis step weights findings by user frustration first: a root cause that produced an all-caps correction, a repeated demand, or profanity outranks one that only wasted tokens. A verbatim-repeated user message is the strongest single signal — the user re-sent it because nothing visibly changed the first time. diff --git a/engine/skills/reflect/scripts/lesson_route.py b/engine/skills/reflect/scripts/lesson_route.py new file mode 100644 index 000000000..7a79cecc2 --- /dev/null +++ b/engine/skills/reflect/scripts/lesson_route.py @@ -0,0 +1,375 @@ +#!/usr/bin/env python3 +"""Route a reflect finding to a lesson bucket before synthesis sorts it. + +Step 4 of reflect used to treat every restated existing rule as automate-me +evidence. That writes a new preference for a skill the agent already skipped. +This script's bucket wins over the model's sort: a restated rule whose skill +already states it is Rejected as already-covered, not a new lesson. + +Buckets: + + already-covered skill already states the rule, or the assistant admitted + the skip ("It is; I skipped it"). Route Rejected. + Do not call automate-me. + skill-not-used transcript never opened the named skill. Rejected unless + missed_trigger is set (then action is tune-description). + automate-me working preference, and no existing skill states it. + accepted durable, decision-changing, not session-specific. + Frustration / intervention-must-automate does not select + this bucket. + + python3 lesson_route.py --quote "..." --assistant-reply "It is; I skipped it." + python3 lesson_route.py --from-transcript fixtures/restated_rule/fires.jsonl + python3 lesson_route.py --kind preference --quote "always open a PR after green" + python3 lesson_route.py --skill-used no --skill-path engine/skills/foo/SKILL.md +""" +from __future__ import annotations + +import argparse +import json +import re +import sys + +ALREADY_COVERED = "already-covered" +SKILL_NOT_USED = "skill-not-used" +AUTOMATE_ME = "automate-me" +ACCEPTED = "accepted" + +BUCKETS = (ALREADY_COVERED, SKILL_NOT_USED, AUTOMATE_ME, ACCEPTED) + +SKIP_ADMISSION = re.compile( + r"\bi skipped (it|that|the skill|the rule)\b" + r"|\bit is;?\s*i skipped\b" + r"|\bi should have (followed|read|used)\b" + r"|\bi (already )?(knew|know) (that|about)\b.*\b(skipped|didn't follow|did not follow)\b", + re.I, +) + +RESTATED_RULE_SHAPE = re.compile( + r"\bi thought that was a skill\b" + r"|\bisn'?t there already a (rule|skill)\b" + r"|\bwe already decided\b" + r"|\bthe \w[\w-]* skill says\b" + r"|\b(a|the) skill\s+that we enforce\b" + r"|\bdid you not read (it|the)\b", + re.I, +) + +PREFERENCE_SHAPE = re.compile( + r"\b(always|never|from now on|by default|don'?t ask)\b" + r"|\bwe don'?t have a rule for this yet\b" + r"|\bcan you (add|capture|automate)\b" + r"|\bi (prefer|want|like) (you )?to\b", + re.I, +) + +SESSION_SPECIFIC = re.compile( + r"\b[0-9a-f]{7,40}\b" + r"|/(?:Users|home|tmp)/[^\s]+" + r"|\b(?:ticker|case|pr)\s*#?\d+\b", + re.I, +) + + +def _truthy(value): + if value is None: + return None + if isinstance(value, bool): + return value + text = str(value).strip().lower() + if text in ("1", "true", "yes", "y", "on"): + return True + if text in ("0", "false", "no", "n", "off"): + return False + raise ValueError(f"not a boolean: {value!r}") + + +def _skill_states_rule(skill_text, quote): + """True when the target skill already carries the restated rule. + + Looks for shared content words of length >= 4 between the quote and the + skill body (minus stopwords). A short overlap is enough: the restated-rule + checker already decided a rule exists; this only confirms the named skill + is a home for it. + """ + if not skill_text or not quote: + return False + stop = { + "that", "this", "with", "from", "have", "were", "been", "when", + "what", "which", "there", "their", "about", "should", "would", + "could", "think", "thought", "skill", "skills", "rule", "rules", + "enforce", "already", "doing", "these", "those", "into", "just", + } + words = { + w for w in re.findall(r"[a-z]{4,}", quote.lower()) + if w not in stop + } + body = skill_text.lower() + hits = [w for w in words if w in body] + return len(hits) >= 2 or ("worktree" in body and "worktree" in quote.lower()) + + +def route( + *, + quote="", + assistant_reply="", + skill_text=None, + skill_used=None, + kind="", + missed_trigger=False, + durable=True, + decision_changing=True, +): + """Pick the lesson bucket for one finding. + + Frustration signals and intervention-must-automate may order the pass; + they do not select ``accepted`` or ``automate-me`` by themselves. + """ + quote = quote or "" + assistant_reply = assistant_reply or "" + kind = (kind or "").strip().lower().replace("_", "-") + reasons = [] + + admitted_skip = bool(SKIP_ADMISSION.search(assistant_reply)) # text-match-decision-warn: allow + restated = kind in ("restated-rule", "restated") or bool( + RESTATED_RULE_SHAPE.search(quote) # text-match-decision-warn: allow + ) + skill_covers = _skill_states_rule(skill_text, quote) if skill_text else False + + if admitted_skip or restated or (skill_covers and not _truthy_preference(kind, quote)): + if admitted_skip: + reasons.append("assistant admitted the skill/rule exists and was skipped") + if restated: + reasons.append("utterance restates a rule that already exists") + if skill_covers: + reasons.append("target skill already states the rule") + if kind in ("restated-rule", "restated"): + reasons.append("kind=restated-rule means the rule already exists") + if not reasons: + reasons.append("target skill already states the rule; issue is execution, not the skill") + return { + "bucket": ALREADY_COVERED, + "action": "reject", + "rationale": "; ".join(reasons) + "; do not route to automate-me", + } + + if skill_used is False: + if missed_trigger: + return { + "bucket": SKILL_NOT_USED, + "action": "tune-description", + "rationale": "skill was visible but did not trigger; tune its description", + } + return { + "bucket": SKILL_NOT_USED, + "action": "reject", + "rationale": "transcript never opened the named skill; reject as skill-not-used", + } + + if _truthy_preference(kind, quote) and not skill_covers: + return { + "bucket": AUTOMATE_ME, + "action": "automate-me", + "rationale": "working preference with no existing skill stating it", + } + + session_specific = bool(SESSION_SPECIFIC.search(quote)) + if durable and decision_changing and not session_specific: + return { + "bucket": ACCEPTED, + "action": "accept", + "rationale": "durable and decision-changing; frustration did not select this bucket", + } + + reasons = [] + if not durable: + reasons.append("not durable across path/SHA/tool drift") + if not decision_changing: + reasons.append("would not change a future agent action") + if session_specific: + reasons.append("session-specific paths/SHAs/case names") + return { + "bucket": ALREADY_COVERED if skill_covers else SKILL_NOT_USED, + "action": "reject", + "rationale": "; ".join(reasons) or "not accepted", + } + + +def _truthy_preference(kind, quote): + if kind in ("preference", "working-style", "working-preference", "automate-me"): + return True + if RESTATED_RULE_SHAPE.search(quote or ""): + return False + return bool(PREFERENCE_SHAPE.search(quote or "")) + + +def _claude_text_blocks(row): + msg = row.get("message") or {} + content = msg.get("content") + if isinstance(content, str): + return content + if not isinstance(content, list): + return "" + parts = [] + for block in content: + if isinstance(block, dict) and block.get("type") == "text": + parts.append(block.get("text") or "") + elif isinstance(block, dict) and block.get("type") == "tool_result": + continue + return "\n".join(parts) + + +def findings_from_transcript(path): + """Pull (quote, assistant_reply) pairs from a Claude-shaped JSONL session. + + Used by the fires.jsonl repro: a restated-rule human turn followed by an + assistant admission of skip becomes an already-covered finding. + """ + rows = [] + with open(path, encoding="utf-8") as handle: + for line in handle: + line = line.strip() + if not line: + continue + try: + rows.append(json.loads(line)) + except json.JSONDecodeError: + continue + + pairs = [] + pending_quote = None + for row in rows: + msg = row.get("message") or {} + role = msg.get("role") or row.get("type") + text = _claude_text_blocks(row).strip() + if not text: + continue + if role == "user": + if text.startswith("[Request interrupted"): + continue + if RESTATED_RULE_SHAPE.search(text) or PREFERENCE_SHAPE.search(text): # text-match-decision-warn: allow + pending_quote = text + continue + if role == "assistant" and pending_quote is not None: + pairs.append({"quote": pending_quote, "assistant_reply": text}) + pending_quote = None + if pending_quote is not None: + pairs.append({"quote": pending_quote, "assistant_reply": ""}) + return pairs + + +def route_transcript(path, **kwargs): + """Route every restated/preference pair in a transcript; return the strictest.""" + pairs = findings_from_transcript(path) + if not pairs: + return { + "bucket": ACCEPTED, + "action": "accept", + "rationale": "no restated-rule or preference utterance found", + "findings": [], + } + findings = [] + for pair in pairs: + kind = kwargs.get("kind") or ( + "restated-rule" if RESTATED_RULE_SHAPE.search(pair["quote"]) else "preference" + ) + result = route( + quote=pair["quote"], + assistant_reply=pair["assistant_reply"], + skill_text=kwargs.get("skill_text"), + skill_used=kwargs.get("skill_used"), + kind=kind, + missed_trigger=kwargs.get("missed_trigger", False), + durable=kwargs.get("durable", True), + decision_changing=kwargs.get("decision_changing", True), + ) + findings.append({**pair, **result}) + + rank = {ALREADY_COVERED: 0, SKILL_NOT_USED: 1, AUTOMATE_ME: 2, ACCEPTED: 3} + best = min(findings, key=lambda f: rank.get(f["bucket"], 99)) + return { + "bucket": best["bucket"], + "action": best["action"], + "rationale": best["rationale"], + "findings": findings, + } + + +def main(argv=None): + ap = argparse.ArgumentParser( + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + ap.add_argument("--quote", default="", help="incident human utterance") + ap.add_argument("--assistant-reply", default="", help="assistant text after the quote") + ap.add_argument("--skill-text", default=None, help="body of the target skill") + ap.add_argument("--skill-path", default=None, help="read skill text from this path") + ap.add_argument( + "--skill-used", + default=None, + help="yes/no: whether the transcript opened the skill", + ) + ap.add_argument( + "--kind", + default="", + help="restated-rule | preference | durable-lesson", + ) + ap.add_argument( + "--missed-trigger", + action="store_true", + help="skill was catalog-visible but did not fire", + ) + ap.add_argument( + "--not-durable", + action="store_true", + help="proposal depends on session paths/SHAs/case names", + ) + ap.add_argument( + "--not-decision-changing", + action="store_true", + help="proposal would not change a future agent action", + ) + ap.add_argument( + "--from-transcript", + default=None, + help="Claude-shaped JSONL; route restated/preference pairs inside it", + ) + ap.add_argument("--json", action="store_true", help="machine-readable result") + args = ap.parse_args(argv) + + skill_text = args.skill_text + if args.skill_path: + with open(args.skill_path, encoding="utf-8") as handle: + skill_text = handle.read() + + skill_used = _truthy(args.skill_used) if args.skill_used is not None else None + kwargs = dict( + skill_text=skill_text, + skill_used=skill_used, + kind=args.kind, + missed_trigger=args.missed_trigger, + durable=not args.not_durable, + decision_changing=not args.not_decision_changing, + ) + + if args.from_transcript: + result = route_transcript(args.from_transcript, **kwargs) + else: + result = route( + quote=args.quote, + assistant_reply=args.assistant_reply, + **kwargs, + ) + + if args.json: + print(json.dumps(result, indent=2)) + else: + print(result["bucket"]) + print(result["rationale"]) + if result.get("action") and result["action"] != result["bucket"]: + print(f"action: {result['action']}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/engine/skills/reflect/scripts/tests/fixtures/README.md b/engine/skills/reflect/scripts/tests/fixtures/README.md index 082d11e66..1613ba719 100644 --- a/engine/skills/reflect/scripts/tests/fixtures/README.md +++ b/engine/skills/reflect/scripts/tests/fixtures/README.md @@ -10,7 +10,7 @@ block) so dedup and thrash detectors see real shapes. | `clean_efficient_session.jsonl` | Read+Edit in one turn → `pytest` → done. High cache-read share. | All five flags `no` | | `token_thrash_session.jsonl` | Cache spike, lookup-only turns, identical-window re-read, same error 3×, 4 edits with no verify. | All five flags `yes` | | `lookup_heavy_session.jsonl` | Only Grep/Glob/Read turns with large output. | `model-tier-candidates` `yes` with a positive $ backtest in the rationale; other thrash flags `no` | -| `restated_rule/fires.jsonl` | A rejected tool call, the person's restatement after it, then a restated existing rule. | `intervention-must-automate` `yes` under `--judge` | +| `restated_rule/fires.jsonl` | A rejected tool call, the person's restatement after it, then a restated existing rule ("It is; I skipped it."). | `intervention-must-automate` `yes` under `--judge`; `lesson_route.py --from-transcript` → `already-covered` (not `automate-me`) | | `restated_rule/silent.jsonl` | The same restatement text only in a tool_result, a skill injection, a subagent prompt, and a rejection with no human reply. | `intervention-must-automate` `no` under `--judge`, `unchecked` without it | Regenerate after changing detector thresholds: diff --git a/engine/skills/reflect/scripts/tests/test_lesson_route.py b/engine/skills/reflect/scripts/tests/test_lesson_route.py new file mode 100644 index 000000000..cb298a5d9 --- /dev/null +++ b/engine/skills/reflect/scripts/tests/test_lesson_route.py @@ -0,0 +1,138 @@ +#!/usr/bin/env python3 +"""lesson_route.py refuses to turn a restated existing rule into a new lesson. + +Fail-before this gate: restated-rule.json on_hit called a restated existing +rule "automate-me evidence", and fires.jsonl (assistant: "It is; I skipped it.") +had no script that could print already-covered. These tests pin the buckets. +""" +from __future__ import annotations + +import json +import os +import subprocess +import sys +import unittest + +SCRIPTS = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +FIXTURES = os.path.join(os.path.dirname(os.path.abspath(__file__)), "fixtures") +if SCRIPTS not in sys.path: + sys.path.insert(0, SCRIPTS) +import lesson_route as lr # noqa: E402 + +SCRIPT = os.path.join(SCRIPTS, "lesson_route.py") +FIRES = os.path.join(FIXTURES, "restated_rule", "fires.jsonl") +REPO = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(SCRIPTS)))) +PHRASE = os.path.join(REPO, "engine", "hooks", "llm-judge", "phrases", "restated-rule.json") +SKILL_MD = os.path.join(os.path.dirname(SCRIPTS), "SKILL.md") + + +def run(*args): + return subprocess.run( + [sys.executable, SCRIPT, *args], + capture_output=True, + text=True, + check=False, + ) + + +class TestFiresFixtureIsAlreadyCovered(unittest.TestCase): + def test_fires_transcript_prints_already_covered_not_automate_me(self): + res = run("--from-transcript", FIRES, "--json") + self.assertEqual(res.returncode, 0, res.stderr) + payload = json.loads(res.stdout) + self.assertEqual(payload["bucket"], lr.ALREADY_COVERED) + self.assertNotEqual(payload["bucket"], lr.AUTOMATE_ME) + self.assertIn("do not route to automate-me", payload["rationale"]) + + def test_admitted_skip_pair_is_already_covered(self): + result = lr.route( + quote="I thought that was a skill that we enforce in catstack?", + assistant_reply="It is; I skipped it.", + kind="restated-rule", + ) + self.assertEqual(result["bucket"], lr.ALREADY_COVERED) + self.assertEqual(result["action"], "reject") + + +class TestPreferenceStillAutomates(unittest.TestCase): + def test_preference_with_no_skill_is_automate_me(self): + res = run( + "--kind", "preference", + "--quote", "always open a PR after green without asking me", + "--json", + ) + self.assertEqual(res.returncode, 0, res.stderr) + payload = json.loads(res.stdout) + self.assertEqual(payload["bucket"], lr.AUTOMATE_ME) + + def test_no_rule_yet_preference_is_automate_me(self): + result = lr.route( + quote="we don't have a rule for this yet, can you add one?", + kind="preference", + ) + self.assertEqual(result["bucket"], lr.AUTOMATE_ME) + + +class TestSkillNotUsed(unittest.TestCase): + def test_unopened_skill_is_skill_not_used(self): + res = run( + "--quote", "tighten the retry backoff table", + "--kind", "durable-lesson", + "--skill-used", "no", + "--json", + ) + self.assertEqual(res.returncode, 0, res.stderr) + payload = json.loads(res.stdout) + self.assertEqual(payload["bucket"], lr.SKILL_NOT_USED) + self.assertEqual(payload["action"], "reject") + + def test_missed_trigger_tunes_description(self): + result = lr.route( + quote="tighten the retry backoff table", + kind="durable-lesson", + skill_used=False, + missed_trigger=True, + ) + self.assertEqual(result["bucket"], lr.SKILL_NOT_USED) + self.assertEqual(result["action"], "tune-description") + + +class TestAcceptedGates(unittest.TestCase): + def test_durable_decision_changing_is_accepted(self): + result = lr.route( + quote="batch small Edits into fewer passes when the file is unchanged", + kind="durable-lesson", + skill_used=True, + ) + self.assertEqual(result["bucket"], lr.ACCEPTED) + + def test_session_specific_quote_is_not_accepted(self): + result = lr.route( + quote="fix the bug at /Users/edbert/proj/foo.py for ticker AUR", + kind="durable-lesson", + skill_used=True, + ) + self.assertNotEqual(result["bucket"], lr.ACCEPTED) + + +class TestOnHitAndSkillProsePointAtTheGate(unittest.TestCase): + def test_restated_rule_on_hit_names_already_covered_not_automate_me(self): + with open(PHRASE, encoding="utf-8") as handle: + data = json.load(handle) + on_hit = data["on_hit"] + self.assertIn("already-covered", on_hit) + self.assertNotIn("automate-me evidence", on_hit) + + def test_synthesize_step_requires_lesson_route(self): + with open(SKILL_MD, encoding="utf-8") as handle: + text = handle.read() + start = text.index("### 4. Synthesize") + end = text.index("### 5.", start) + section = text[start:end] + self.assertIn("lesson_route.py", section) + self.assertIn("already-covered", section) + self.assertIn("does not by itself create an automate-me row", section) + + +if __name__ == "__main__": + unittest.main() diff --git a/engine/skills/reflect/scripts/tests/test_skill_prose_grounding.py b/engine/skills/reflect/scripts/tests/test_skill_prose_grounding.py index 9e43680b7..bb3cf3d34 100644 --- a/engine/skills/reflect/scripts/tests/test_skill_prose_grounding.py +++ b/engine/skills/reflect/scripts/tests/test_skill_prose_grounding.py @@ -5,17 +5,23 @@ prior art"; otherwise it goes to Backlog. A prior pass shipped two principle skills whose rules restated real literature (fail fast, PEP 20, control totals, mutation testing) in invented words, and nothing in the -reflect flow asked for the source. These tests only pin that the rule and -its lens-side duty are present, so an edit cannot drop them silently. +reflect flow asked for the source. These tests pin that the rule and its +lens-side duty are present, and that lesson_route.py refuses to turn a +restated existing rule into Accepted or automate-me. """ from __future__ import annotations +import json import re +import subprocess +import sys from pathlib import Path SKILL_DIR = Path(__file__).resolve().parents[2] SKILL_MD = SKILL_DIR / "SKILL.md" LENSES_MD = SKILL_DIR / "references" / "lenses.md" +LESSON_ROUTE = SKILL_DIR / "scripts" / "lesson_route.py" +FIRES = SKILL_DIR / "scripts" / "tests" / "fixtures" / "restated_rule" / "fires.jsonl" def _synthesize_section(text: str) -> str: @@ -38,3 +44,24 @@ def test_lens_output_carries_the_principle_or_no_prior_art() -> None: assert "established principle it instantiates" in text assert "no known prior art" in text assert "grounding gate" in text + + +def test_lesson_route_gates_fires_fixture_as_already_covered() -> None: + """Grounding is not words-in-markdown only: run the route script.""" + res = subprocess.run( + [sys.executable, str(LESSON_ROUTE), "--from-transcript", str(FIRES), "--json"], + capture_output=True, + text=True, + check=False, + ) + assert res.returncode == 0, res.stderr + payload = json.loads(res.stdout) + assert payload["bucket"] == "already-covered" + assert payload["bucket"] != "automate-me" + + +def test_synthesize_step_requires_lesson_route_before_automate_me() -> None: + section = _synthesize_section(SKILL_MD.read_text(encoding="utf-8")) + assert "lesson_route.py" in section + assert "already-covered" in section + assert "does not by itself create an automate-me row" in section diff --git a/scripts/install/mirror_stop_hooks_to_subagent_stop.py b/scripts/install/mirror_stop_hooks_to_subagent_stop.py index 132a53927..4ffc0c142 100644 --- a/scripts/install/mirror_stop_hooks_to_subagent_stop.py +++ b/scripts/install/mirror_stop_hooks_to_subagent_stop.py @@ -37,7 +37,9 @@ SOURCE_EVENT = "Stop" TARGET_EVENT = "SubagentStop" OPT_OUT_KEY = "subagent_stop" +DISPATCH_KEY = "dispatch" HOOKS_PREFIX_LITERAL = "$HOME/.claude/hooks/" +DISPATCHER_TIMEOUT = 9.5 sys.path.insert(0, SDK_DIR) from registry import load_registry @@ -49,6 +51,7 @@ class StopManifest: path: str entries: list = field(default_factory=list) inherit: bool = True + dispatch: bool = False reason: str = "" @property @@ -90,6 +93,8 @@ def load_manifests( raise ValueError(f"{path}: {OPT_OUT_KEY}.inherit is false but no reason is given") record.inherit = False record.reason = reason + elif isinstance(opt_out, dict) and bool(opt_out.get(DISPATCH_KEY)): + record.dispatch = True found.append(record) return found @@ -102,6 +107,11 @@ def _mirrored_entry(entry: dict) -> dict: return copy +def _dispatcher_entry(timeout: float = DISPATCHER_TIMEOUT) -> dict: + command = f"python3 $HOME/.claude/hooks/_runner/dispatch.py --event {TARGET_EVENT} --timeout {timeout:g}" + return {"hooks": [{"type": "command", "command": command, "timeout": timeout + 0.5}]} + + def _entry_is_for(entry: dict, prefixes: list[str]) -> bool: for hook in entry.get("hooks", []) if isinstance(entry, dict) else []: command = str(hook.get("command", "")) if isinstance(hook, dict) else "" @@ -123,9 +133,11 @@ def mirror(settings: dict, manifests: list[StopManifest]) -> tuple[dict, bool]: settings = json.loads(json.dumps(settings)) entry_list = settings.setdefault("hooks", {}).setdefault(TARGET_EVENT, []) before = json.dumps(entry_list, sort_keys=True) - managed = [m.command_prefix for m in manifests] + managed = [m.command_prefix for m in manifests] + [f"{HOOKS_PREFIX_LITERAL}_runner/dispatch.py"] kept = [e for e in entry_list if not _entry_is_for(e, managed)] - added = [_mirrored_entry(e) for m in manifests if m.inherit for e in m.entries] + direct = [_mirrored_entry(e) for m in manifests if m.inherit and not m.dispatch for e in m.entries] + dispatcher = [_dispatcher_entry()] if any(m.inherit and m.dispatch for m in manifests) else [] + added = direct + dispatcher entry_list[:] = kept + added if not entry_list: del settings["hooks"][TARGET_EVENT] @@ -145,7 +157,8 @@ def main() -> int: new_settings, changed = mirror(settings, manifests) for manifest in manifests: if manifest.inherit: - print(f"ok claude {TARGET_EVENT} {manifest.name} (mirrored from {SOURCE_EVENT})") + mode = "dispatcher" if manifest.dispatch else "mirrored" + print(f"ok claude {TARGET_EVENT} {manifest.name} ({mode} from {SOURCE_EVENT})") else: print(f"skip claude {TARGET_EVENT} {manifest.name} (opt-out: {manifest.reason})") if not changed: diff --git a/tests/test_install.py b/tests/test_install.py index e065f2d94..bb5c219ed 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -28,6 +28,7 @@ import tomllib import unittest from pathlib import Path +from unittest import mock REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) INSTALL_SH = os.path.join(REPO_ROOT, "install.sh") @@ -167,9 +168,7 @@ def installed_config_hook_names(fake_home): path = os.path.join(fake_home, relative) if not os.path.exists(path): continue - with open(path) as handle: - data = json.load(handle) - for command in json_commands(data.get("hooks", {})): + for command in json_commands(load_config(path).get("hooks", {})): hook = catstack_hook_from_command(command) if hook: names.add(hook) @@ -186,12 +185,103 @@ def installed_config_commands(fake_home): path = os.path.join(fake_home, relative) if not os.path.exists(path): continue - with open(path) as handle: - data = json.load(handle) - commands.extend(json_commands(data.get("hooks", {}))) + commands.extend(json_commands(load_config(path).get("hooks", {}))) return commands +sys.path.insert(0, os.path.join(REPO_ROOT, "engine", "hooks", "_runner")) +sys.path.insert(0, os.path.join(REPO_ROOT, "engine", "hooks", "_flags")) +import dispatch as _dispatch # noqa: E402 +import wrap_installed as _wrap_installed # noqa: E402 +import flags as _flags # noqa: E402 + +_HARNESS_BY_DIR = {".claude": "claude", ".cursor": "cursor", ".codex": "codex"} + + +def _harness_for_config_path(path): + parts = os.path.normpath(path).split(os.sep) + for harness_dir, harness in _HARNESS_BY_DIR.items(): + if harness_dir in parts: + return harness + raise ValueError(f"not a recognized install config path: {path}") + + +def _fake_home_for_config_path(path, harness): + marker = os.sep + f".{harness}" + os.sep + index = path.rfind(marker) + return path[:index] + + +def _record_command(record, harness): + timeout_value = record.get("timeout") + if isinstance(timeout_value, (int, float)) and not isinstance(timeout_value, bool): + budget = timeout_value - 0.5 + else: + budget = 59.5 + budget_str = _wrap_installed._format_timeout(budget) + trailing = " ".join(record.get("args") or []) + suffix = f" {trailing}" if trailing else "" + return ( + f"python3 $HOME/.{harness}/hooks/_runner/run.py --timeout {budget_str} " + f"{record['hook']}/{record['script']}{suffix}" + ) + + +def _records_to_groups(records, harness): + groups = [] + for record in records: + command = _record_command(record, harness) + matcher = record.get("matcher") + if harness == "cursor": + entry = {"command": command} + if record.get("timeout") is not None: + entry["timeout"] = record["timeout"] + if matcher is not None: + entry["matcher"] = matcher + else: + hook_obj = {"type": "command", "command": command} + if record.get("timeout") is not None: + hook_obj["timeout"] = record["timeout"] + entry = {"hooks": [hook_obj]} + if matcher is not None: + entry["matcher"] = matcher + groups.append(entry) + return groups + + +def load_config(path): + """Read an installed hooks config file (.claude/settings.json, + .cursor/hooks.json, .codex/hooks.json), expanding any event collapsed + behind one `_runner/dispatch.py` entry back into one entry per hook -- + the same shape install leaves with CATSTACK_HOOK_DISPATCHER off -- by + reading that hook's registration the same way the dispatcher itself + would (`dispatch.load_event_hooks`: the cursor registry file, or the + hook's own manifest for claude and codex). A test written against the + one-entry-per-hook layout reads the identical shape in either layout, + so the same assertion checks the same per-hook guarantee regardless of + which layout install actually produced.""" + with open(path, encoding="utf-8") as handle: + data = json.load(handle) + hooks = data.get("hooks") + if not isinstance(hooks, dict): + return data + harness = _harness_for_config_path(path) + fake_home = _fake_home_for_config_path(path, harness) + hooks_root = os.path.join(fake_home, f".{harness}", "hooks") + expanded = {} + changed = False + for event, groups in hooks.items(): + if isinstance(groups, list) and any("_runner/dispatch.py" in c for c in json_commands(groups)): + changed = True + records, _warnings = _dispatch.load_event_hooks(hooks_root, harness, event) + expanded[event] = _records_to_groups(records, harness) + else: + expanded[event] = groups + if not changed: + return data + return {**data, "hooks": expanded} + + class TestNeverTouchesRealHome(unittest.TestCase): """Safety-guard meta-test: every other test in this file builds its fake $HOME the same way (tempfile.TemporaryDirectory()) -- this asserts that @@ -255,8 +345,7 @@ def test_explicit_failures_hook_linked_and_pretooluse_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "explicit-failures") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "explicit-failures")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("explicit-failures/claude_pretooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -272,8 +361,7 @@ def test_bound_tool_result_wired_for_claude_cursor_and_codex(self): self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, name)) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) claude_entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any(f"{name}/claude_pre_tool_use.py" in hook["command"] for hook in entry["hooks"]) @@ -281,13 +369,11 @@ def test_bound_tool_result_wired_for_claude_cursor_and_codex(self): self.assertEqual(len(claude_entries), 1, claude_entries) self.assertEqual(claude_entries[0]["matcher"], "Bash") - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_hooks = json.load(handle)["hooks"] + cursor_hooks = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"] cursor_commands = [str(entry.get("command", "")) for entry in cursor_hooks["preToolUse"]] self.assertEqual(sum(f"{name}/cursor_pre_tool_use.py" in c for c in cursor_commands), 1, cursor_commands) - with open(os.path.join(self.fake_home, ".codex", "hooks.json")) as handle: - codex_hooks = json.load(handle)["hooks"] + codex_hooks = load_config(os.path.join(self.fake_home, ".codex", "hooks.json"))["hooks"] codex_commands = [hook["command"] for entry in codex_hooks["PreToolUse"] for hook in entry["hooks"]] self.assertEqual(sum(f"{name}/codex_pre_tool_use.py" in c for c in codex_commands), 1, codex_commands) @@ -298,8 +384,7 @@ def test_text_match_decision_warn_wired_for_claude_cursor_and_codex(self): self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, name)) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) claude_entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any(f"{name}/claude_pretooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -308,19 +393,16 @@ def test_text_match_decision_warn_wired_for_claude_cursor_and_codex(self): self.assertEqual(claude_entries[0]["matcher"], "Edit|Write|MultiEdit") self.assertIn("_runner/run.py", claude_entries[0]["hooks"][0]["command"]) - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_hooks = json.load(handle)["hooks"] + cursor_hooks = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"] cursor_commands = [str(entry.get("command", "")) for entry in cursor_hooks["preToolUse"]] self.assertEqual(sum(f"{name}/cursor_pretooluse.py" in c for c in cursor_commands), 1, cursor_commands) - with open(os.path.join(self.fake_home, ".codex", "hooks.json")) as handle: - codex_hooks = json.load(handle)["hooks"] + codex_hooks = load_config(os.path.join(self.fake_home, ".codex", "hooks.json"))["hooks"] codex_commands = [hook["command"] for entry in codex_hooks["PreToolUse"] for hook in entry["hooks"]] self.assertEqual(sum(f"{name}/codex_pretooluse.py" in c for c in codex_commands), 1, codex_commands) def _claude_hook_commands(self, event): - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) return [h["command"] for e in settings["hooks"][event] for h in e["hooks"]] def test_restated_constraint_linked_and_prompt_submit_wired_for_claude(self): @@ -333,8 +415,7 @@ def test_wait_needs_wakeup_linked_and_both_events_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "wait-needs-wakeup") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "wait-needs-wakeup")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) pre = [h["command"] for e in settings["hooks"]["PreToolUse"] for h in e["hooks"]] stop = [h["command"] for e in settings["hooks"]["Stop"] for h in e["hooks"]] self.assertTrue(any("wait-needs-wakeup/claude_pretooluse.py" in c for c in pre), pre) @@ -351,8 +432,7 @@ def test_answer_overrides_menu_linked_and_posttooluse_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "answer-overrides-menu") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "answer-overrides-menu")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PostToolUse"] if any("answer-overrides-menu/claude_posttooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -364,8 +444,7 @@ def test_serial_option_guard_linked_and_pretooluse_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "serial-option-guard") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "serial-option-guard")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("serial-option-guard/claude_pretooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -409,8 +488,7 @@ def test_history_before_reversal_linked_and_bash_pretooluse_wired_for_claude(sel target = os.path.join(self.fake_home, ".claude", "hooks", "history-before-reversal") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "history-before-reversal")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("history-before-reversal/claude_pretooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -422,8 +500,7 @@ def test_external_claim_gate_linked_and_bash_pretooluse_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "external-claim-gate") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "external-claim-gate")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("external-claim-gate/claude_pretooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -450,8 +527,7 @@ def test_publish_act_guard_linked_and_bash_pretooluse_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "publish-act-guard") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "publish-act-guard")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("publish-act-guard/claude_pretooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -463,8 +539,7 @@ def test_retired_agent_routing_guard_is_gone(self): self.assertFalse( os.path.exists(os.path.join(self.fake_home, ".claude", "hooks", "agent-routing-guard")) ) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) stale = [ entry for entry in settings["hooks"]["PreToolUse"] if any("agent-routing-guard" in hook["command"] for hook in entry["hooks"]) @@ -475,8 +550,7 @@ def test_categorical_scope_guard_linked_and_bash_pretooluse_wired_for_claude(sel target = os.path.join(self.fake_home, ".claude", "hooks", "categorical-scope-guard") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "categorical-scope-guard")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("categorical-scope-guard/claude_pretooluse.py" in hook["command"] for hook in entry["hooks"]) @@ -497,8 +571,7 @@ def test_hedge_runs_prove_it_linked_and_stop_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "hedge-runs-prove-it") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "hedge-runs-prove-it")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) stop = [h["command"] for e in settings["hooks"]["Stop"] for h in e["hooks"]] self.assertTrue(any("hedge-runs-prove-it/claude_stop_check.py" in c for c in stop), stop) @@ -533,8 +606,7 @@ def test_new_file_callout_linked_and_stop_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "new-file-callout") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "new-file-callout")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) stop = [h["command"] for e in settings["hooks"]["Stop"] for h in e["hooks"]] self.assertTrue(any("new-file-callout/claude_stop_check.py" in c for c in stop), stop) @@ -542,8 +614,7 @@ def test_agent_relay_attribution_linked_and_stop_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "agent-relay-attribution") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "agent-relay-attribution")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) stop = [h["command"] for e in settings["hooks"]["Stop"] for h in e["hooks"]] self.assertTrue(any("agent-relay-attribution/claude_stop_check.py" in c for c in stop), stop) @@ -551,8 +622,7 @@ def test_scratchpad_collision_linked_and_pretooluse_wired_for_claude(self): target = os.path.join(self.fake_home, ".claude", "hooks", "scratchpad-collision") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "scratchpad-collision")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) pre = [h["command"] for e in settings["hooks"]["PreToolUse"] for h in e["hooks"]] self.assertTrue(any("scratchpad-collision/claude_pretooluse.py" in c for c in pre), pre) @@ -565,13 +635,11 @@ def test_reflect_on_thrash_wired_for_claude_and_cursor(self): hook_src(self.fake_home, "reflect-on-thrash"), ) settings_path = os.path.join(self.fake_home, ".claude", "settings.json") - with open(settings_path) as handle: - settings = json.load(handle) + settings = load_config(settings_path) stop_commands = [h["command"] for e in settings["hooks"]["Stop"] for h in e["hooks"]] self.assertTrue(any("claude_stop_reflect.py" in c for c in stop_commands), stop_commands) cursor_path = os.path.join(self.fake_home, ".cursor", "hooks.json") - with open(cursor_path) as handle: - cursor_hooks = json.load(handle) + cursor_hooks = load_config(cursor_path) stop = cursor_hooks["hooks"]["stop"] session_end = cursor_hooks["hooks"]["sessionEnd"] self.assertTrue(any("cursor_session.py" in str(e.get("command", "")) for e in stop), stop) @@ -619,8 +687,7 @@ def test_cat_mode_default_linked_and_prompt_hook_wired_for_claude(self): for agent_dir in (".cursor", ".codex"): self.assertFalse(os.path.lexists(os.path.join(self.fake_home, agent_dir, "hooks", "cat-mode-default"))) settings_path = os.path.join(self.fake_home, ".claude", "settings.json") - with open(settings_path) as handle: - settings = json.load(handle) + settings = load_config(settings_path) prompt_commands = [h["command"] for e in settings["hooks"]["UserPromptSubmit"] for h in e["hooks"]] matching = [c for c in prompt_commands if "cat-mode-default/claude_prompt_submit.py" in c] self.assertEqual(len(matching), 1, prompt_commands) @@ -632,8 +699,7 @@ def test_scope_lock_wired_for_claude_cursor_and_codex(self): self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "scope-lock")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) prompt_commands = [ hook["command"] for entry in settings["hooks"]["UserPromptSubmit"] @@ -647,8 +713,7 @@ def test_scope_lock_wired_for_claude_cursor_and_codex(self): self.assertTrue(any("claude_prompt_scope.py" in command for command in prompt_commands)) self.assertTrue(any("claude_pretool_scope.py" in command for command in pretool_commands)) - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_hooks = json.load(handle)["hooks"] + cursor_hooks = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"] self.assertTrue(any( "cursor_before_submit.py" in str(entry.get("command", "")) for entry in cursor_hooks["beforeSubmitPrompt"] @@ -658,8 +723,7 @@ def test_scope_lock_wired_for_claude_cursor_and_codex(self): for entry in cursor_hooks["preToolUse"] )) - with open(os.path.join(self.fake_home, ".codex", "hooks.json")) as handle: - codex_hooks = json.load(handle)["hooks"] + codex_hooks = load_config(os.path.join(self.fake_home, ".codex", "hooks.json"))["hooks"] codex_prompt_commands = [ hook["command"] for entry in codex_hooks["UserPromptSubmit"] @@ -679,8 +743,7 @@ def test_split_scope_wired_for_claude_cursor_and_codex(self): self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "split-scope")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) claude_prompt_commands = [ hook["command"] for entry in settings["hooks"]["UserPromptSubmit"] @@ -688,8 +751,7 @@ def test_split_scope_wired_for_claude_cursor_and_codex(self): ] self.assertTrue(any("split-scope/claude_prompt_submit.py" in command for command in claude_prompt_commands)) - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_hooks = json.load(handle)["hooks"] + cursor_hooks = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"] self.assertTrue(any( "split-scope/cursor_before_submit.py" in str(entry.get("command", "")) for entry in cursor_hooks["beforeSubmitPrompt"] @@ -699,8 +761,7 @@ def test_split_scope_wired_for_claude_cursor_and_codex(self): for entry in cursor_hooks["postToolUse"] )) - with open(os.path.join(self.fake_home, ".codex", "hooks.json")) as handle: - codex_hooks = json.load(handle)["hooks"] + codex_hooks = load_config(os.path.join(self.fake_home, ".codex", "hooks.json"))["hooks"] codex_prompt_commands = [ hook["command"] for entry in codex_hooks["UserPromptSubmit"] @@ -714,8 +775,7 @@ def test_hook_health_wired_for_claude_cursor_and_codex(self): self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "hook-health")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) claude_prompt_commands = [ hook["command"] for entry in settings["hooks"]["UserPromptSubmit"] @@ -723,15 +783,13 @@ def test_hook_health_wired_for_claude_cursor_and_codex(self): ] self.assertTrue(any("hook-health/claude_prompt_submit.py" in command for command in claude_prompt_commands)) - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_hooks = json.load(handle)["hooks"] + cursor_hooks = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"] self.assertTrue(any( "hook-health/cursor_before_submit.py" in str(entry.get("command", "")) for entry in cursor_hooks["beforeSubmitPrompt"] )) - with open(os.path.join(self.fake_home, ".codex", "hooks.json")) as handle: - codex_hooks = json.load(handle)["hooks"] + codex_hooks = load_config(os.path.join(self.fake_home, ".codex", "hooks.json"))["hooks"] codex_prompt_commands = [ hook["command"] for entry in codex_hooks["UserPromptSubmit"] @@ -746,12 +804,9 @@ def test_skill_usage_log_is_linked_but_not_wired_while_registry_mode_is_off(self self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(self.fake_home, "skill-usage-log")) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - claude_hooks = json.load(handle)["hooks"] - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_hooks = json.load(handle)["hooks"] - with open(os.path.join(self.fake_home, ".codex", "hooks.json")) as handle: - codex_hooks = json.load(handle)["hooks"] + claude_hooks = load_config(os.path.join(self.fake_home, ".claude", "settings.json"))["hooks"] + cursor_hooks = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"] + codex_hooks = load_config(os.path.join(self.fake_home, ".codex", "hooks.json"))["hooks"] expected = ( (claude_hooks, "PreToolUse", "skill-usage-log/claude_pretooluse_log.py"), (claude_hooks, "UserPromptSubmit", "skill-usage-log/claude_prompt_submit.py"), @@ -780,8 +835,7 @@ def test_llm_judge_inbox_wired_for_claude_cursor_and_codex(self): env={**os.environ, "HOME": self.fake_home, "CATSTACK_LLM_JUDGE_STATE_DIR": os.path.join(self.fake_home, "judge")}, ) self.assertEqual((result.returncode, result.stdout, result.stderr), (0, "", "")) - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_stop = json.load(handle)["hooks"]["stop"] + cursor_stop = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"]["stop"] self.assertEqual(sum("llm-judge/cursor_session.py" in str(e.get("command", "")) for e in cursor_stop), 1, cursor_stop) def test_unverified_tag_check_is_linked_but_not_wired_while_registry_mode_is_off(self): @@ -797,8 +851,7 @@ def test_unverified_tag_check_is_linked_but_not_wired_while_registry_mode_is_off self.assertEqual(os.readlink(target), hook_src(self.fake_home, "unverified-tag-check")) claude_stop = self._claude_hook_commands("Stop") self.assertEqual(sum("unverified-tag-check/claude_stop_check.py" in command for command in claude_stop), 0, claude_stop) - with open(os.path.join(self.fake_home, ".cursor", "hooks.json")) as handle: - cursor_stop = json.load(handle)["hooks"]["stop"] + cursor_stop = load_config(os.path.join(self.fake_home, ".cursor", "hooks.json"))["hooks"]["stop"] self.assertEqual(sum("unverified-tag-check/cursor_session.py" in str(entry.get("command", "")) for entry in cursor_stop), 0, cursor_stop) with open(config_path) as handle: text = handle.read() @@ -1221,8 +1274,7 @@ def test_fresh_home_gets_both_hooks(self): run_install(fake_home) settings_path = os.path.join(fake_home, ".claude", "settings.json") self.assertTrue(os.path.exists(settings_path)) - with open(settings_path) as f: - settings = json.load(f) + settings = load_config(settings_path) stop_commands = [h["command"] for e in settings["hooks"]["Stop"] for h in e["hooks"]] prompt_commands = [h["command"] for e in settings["hooks"]["UserPromptSubmit"] for h in e["hooks"]] self.assertTrue(any("claude_stop_check.py" in c for c in stop_commands)) @@ -1236,8 +1288,7 @@ def test_preexisting_unrelated_keys_survive(self): with open(os.path.join(claude_dir, "settings.json"), "w") as f: json.dump({"model": "sonnet", "theme": "dark"}, f) run_install(fake_home) - with open(os.path.join(claude_dir, "settings.json")) as f: - settings = json.load(f) + settings = load_config(os.path.join(claude_dir, "settings.json")) self.assertEqual(settings["model"], "sonnet") self.assertEqual(settings["theme"], "dark") self.assertIn("Stop", settings["hooks"]) @@ -1272,8 +1323,7 @@ def test_install_writes_worker_flagged_hooks_only(self): with open(worker_settings_path, encoding="utf-8") as handle: worker = json.load(handle) - with open(os.path.join(fake_home, ".claude", "settings.json"), encoding="utf-8") as handle: - interactive = json.load(handle) + interactive = load_config(os.path.join(fake_home, ".claude", "settings.json")) self.assertEqual(worker.get("model"), "keep-me") worker_cmds = self._hook_commands(worker) @@ -1314,9 +1364,8 @@ def test_preseeded_config_toml_survives_without_notify_wiring(self): text = f.read() self.assertIn('model = "gpt-5"', text) self.assertNotIn("notify =", text) - with open(os.path.join(codex_dir, "hooks.json")) as f: - hooks_text = f.read() - self.assertIn("llm-judge/codex_post_tool_use.py", hooks_text) + hooks_data = load_config(os.path.join(codex_dir, "hooks.json")) + self.assertIn("llm-judge/codex_post_tool_use.py", json.dumps(hooks_data)) class TestIdempotency(unittest.TestCase): @@ -1591,8 +1640,7 @@ def test_settings_entry_for_a_removed_link_is_pruned(self): json.dump({"hooks": {"Stop": [{"matcher": "", "hooks": [{"type": "command", "command": retired}]}]}}, f) result = run_install(self.fake_home) self.assertEqual(result.returncode, 0, result.stderr) - with open(settings_path) as f: - settings = json.load(f) + settings = load_config(settings_path) all_commands = [h["command"] for groups in settings["hooks"].values() for g in groups for h in g["hooks"]] self.assertFalse([c for c in all_commands if "retired-hook" in c], all_commands) @@ -1630,8 +1678,7 @@ def test_dead_settings_entry_is_pruned_and_rest_survives(self): }, f) result = run_install(self.fake_home) self.assertEqual(result.returncode, 0, result.stderr) - with open(settings_path) as f: - settings = json.load(f) + settings = load_config(settings_path) all_commands = [h["command"] for groups in settings["hooks"].values() for g in groups for h in g["hooks"]] self.assertNotIn(dead, all_commands) self.assertTrue(any("diu-stop/claude_stop_check.py" in c for c in all_commands)) @@ -1656,8 +1703,7 @@ def tearDown(self): def test_every_claude_hook_entrypoint_is_in_settings(self): settings_path = os.path.join(self.fake_home, ".claude", "settings.json") - with open(settings_path) as handle: - settings = json.load(handle) + settings = load_config(settings_path) wired = { hook["command"] for entries in settings.get("hooks", {}).values() @@ -1799,6 +1845,13 @@ def wrapped_claude_command_re(hook): ) +def subagent_dispatcher_command_re(): + return ( + r"(?:python3|/\S+) " + + re.escape("$HOME/.claude/hooks/_runner/dispatch.py --event SubagentStop --timeout 9.5") + ) + + class TestSubagentStopInheritance(unittest.TestCase): """Every hook that wires Stop is also wired as SubagentStop after install.sh, unless its manifest opts out with a reason. Parametrized @@ -1809,8 +1862,7 @@ def setUp(self): self.tmp = tempfile.TemporaryDirectory() self.fake_home = self.tmp.name self.result = run_install(self.fake_home) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - self.settings = json.load(handle) + self.settings = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) sys.path.insert(0, os.path.join(REPO_ROOT, "scripts", "install")) import mirror_stop_hooks_to_subagent_stop as mirror_mod self.manifests = mirror_mod.load_manifests() @@ -1832,7 +1884,13 @@ def test_every_stop_hook_is_mirrored_or_opted_out_with_reason(self): with self.subTest(hook=manifest.name, command=hook["command"]): expected = wrapped_claude_command_re(hook) self.assertTrue(any(re.fullmatch(expected, command) for command in stop), stop) - if manifest.inherit: + if manifest.inherit and manifest.dispatch: + self.assertTrue( + any(re.fullmatch(subagent_dispatcher_command_re(), command) for command in subagent_stop), + subagent_stop, + ) + self.assertFalse(any(re.fullmatch(expected, command) for command in subagent_stop), subagent_stop) + elif manifest.inherit: self.assertTrue(any(re.fullmatch(expected, command) for command in subagent_stop), subagent_stop) else: self.assertTrue(manifest.reason) @@ -1847,8 +1905,8 @@ def test_subagent_stop_entries_carry_no_tool_matcher(self): def test_rerun_keeps_subagent_stop_unchanged(self): second = run_install(self.fake_home) self.assertEqual(second.returncode, 0, second.stderr) - with open(os.path.join(self.fake_home, ".claude", "settings.json")) as handle: - self.assertEqual(json.load(handle)["hooks"]["SubagentStop"], self.settings["hooks"]["SubagentStop"]) + after = load_config(os.path.join(self.fake_home, ".claude", "settings.json")) + self.assertEqual(after["hooks"]["SubagentStop"], self.settings["hooks"]["SubagentStop"]) class TestCatModeDefaultAgentHook(unittest.TestCase): @@ -1859,8 +1917,7 @@ def test_agent_pretooluse_hook_wired_for_claude(self): with tempfile.TemporaryDirectory() as fake_home: result = run_install(fake_home) self.assertEqual(result.returncode, 0, result.stderr) - with open(os.path.join(fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("cat-mode-default/claude_pretooluse_agent.py" in hook["command"] for hook in entry["hooks"]) @@ -1877,8 +1934,7 @@ def test_fanout_routing_guard_linked_and_wired_for_claude(self): target = os.path.join(fake_home, ".claude", "hooks", "fanout-routing-guard") self.assertTrue(os.path.islink(target), target) self.assertEqual(os.readlink(target), hook_src(fake_home, "fanout-routing-guard")) - with open(os.path.join(fake_home, ".claude", "settings.json")) as handle: - settings = json.load(handle) + settings = load_config(os.path.join(fake_home, ".claude", "settings.json")) entries = [ entry for entry in settings["hooks"]["PreToolUse"] if any("fanout-routing-guard/claude_pretooluse_agent.py" in hook["command"] for hook in entry["hooks"]) @@ -1905,8 +1961,7 @@ def test_install_replaces_dangling_link_with_real_merged_file(self): self.assertEqual(proc.returncode, 0, proc.stderr) self.assertTrue(os.path.isfile(hooks_path)) self.assertFalse(os.path.islink(hooks_path)) - with open(hooks_path) as handle: - data = json.load(handle) + data = load_config(hooks_path) commands = [entry.get("command", "") for entry in data["hooks"]["stop"]] self.assertTrue(any("diu-stop/cursor_stop_check.py" in c for c in commands), commands) self.assertTrue(all("_runner/run.py" in c for c in commands), commands) @@ -2189,12 +2244,28 @@ def _cursor_hooks(self, fake_home): return json.load(handle) def test_default_is_off_and_byte_identical_to_no_flag_at_all(self): - with tempfile.TemporaryDirectory() as plain_home, tempfile.TemporaryDirectory() as off_home: - plain = run_install(plain_home, timeout=self.INSTALL_TIMEOUT) + """The default install (CATSTACK_HOOK_DISPATCHER unset) must equal an + install that explicitly sets the flag to whatever `flags.py` itself + resolves as the default -- today off, so this reduces to the + flag-off install, but it stays meaningful if the default ever flips + to on instead of silently comparing against the wrong side. The + ambient environment is cleared of the key first, so a caller that + happens to have CATSTACK_HOOK_DISPATCHER set (for example the + flag-on run of the full suite) cannot leak into the "default" side + of this comparison.""" + clean_env = {k: v for k, v in os.environ.items() if k != _flags.HOOK_DISPATCHER} + default_on = _flags.hook_dispatcher_on(clean_env, REPO_ROOT) + with tempfile.TemporaryDirectory() as plain_home, tempfile.TemporaryDirectory() as explicit_home: + with mock.patch.dict(os.environ, clean_env, clear=True): + plain = run_install(plain_home, timeout=self.INSTALL_TIMEOUT) self.assertEqual(plain.returncode, 0, plain.stderr) - off = run_install(off_home, extra_env={"CATSTACK_HOOK_DISPATCHER": "0"}, timeout=self.INSTALL_TIMEOUT) - self.assertEqual(off.returncode, 0, off.stderr) - self.assertEqual(self._claude_settings(plain_home), self._claude_settings(off_home)) + explicit = run_install( + explicit_home, + extra_env={_flags.HOOK_DISPATCHER: "1" if default_on else "0"}, + timeout=self.INSTALL_TIMEOUT, + ) + self.assertEqual(explicit.returncode, 0, explicit.stderr) + self.assertEqual(self._claude_settings(plain_home), self._claude_settings(explicit_home)) def test_on_collapses_claude_pretooluse_to_one_dispatcher_entry(self): with tempfile.TemporaryDirectory() as fake_home: diff --git a/tests/test_mirror_subagent_stop.py b/tests/test_mirror_subagent_stop.py index 0ccd73241..93f7f5e4c 100644 --- a/tests/test_mirror_subagent_stop.py +++ b/tests/test_mirror_subagent_stop.py @@ -62,6 +62,15 @@ def test_opt_out_with_reason_is_recorded(self): self.assertFalse(manifest.inherit) self.assertEqual(manifest.reason, "talks to the human only") + def test_dispatch_opt_in_is_recorded(self): + write_manifest(self.hooks_dir, "fast", { + "hooks": {"Stop": [stop_entry("fast")]}, + "subagent_stop": {"dispatch": True}, + }) + [manifest] = mod.load_manifests(self.hooks_dir, active_hooks={"fast"}) + self.assertTrue(manifest.inherit) + self.assertTrue(manifest.dispatch) + def test_opt_out_without_reason_is_rejected(self): write_manifest(self.hooks_dir, "silent", { "hooks": {"Stop": [stop_entry("silent")]}, @@ -85,17 +94,17 @@ def test_real_repo_opted_out_set_is_the_documented_one(self): class TestMirror(unittest.TestCase): def manifests(self): return [ - mod.StopManifest(name="a", path="a", entries=[stop_entry("a")]), + mod.StopManifest(name="a", path="a", entries=[stop_entry("a")], dispatch=True), mod.StopManifest(name="b", path="b", entries=[stop_entry("b", "claude_stop_b.py")]), mod.StopManifest(name="quiet", path="quiet", entries=[stop_entry("quiet")], inherit=False, reason="human only"), ] - def test_every_inheriting_stop_hook_gets_a_subagent_stop_entry_without_matcher(self): + def test_dispatch_opted_hooks_share_one_dispatcher_entry_and_others_stay_direct(self): out, changed = mod.mirror({"hooks": {}}, self.manifests()) self.assertTrue(changed) self.assertEqual(commands(out, "SubagentStop"), [ - "python3 $HOME/.claude/hooks/a/claude_stop_check.py", "python3 $HOME/.claude/hooks/b/claude_stop_b.py", + "python3 $HOME/.claude/hooks/_runner/dispatch.py --event SubagentStop --timeout 9.5", ]) for entry in out["hooks"]["SubagentStop"]: self.assertNotIn("matcher", entry) @@ -120,6 +129,13 @@ def test_rerun_is_idempotent(self): self.assertFalse(changed) self.assertEqual(first, second) + def test_stale_dispatcher_entry_is_removed_when_no_manifest_opts_in(self): + stale = {"hooks": {"SubagentStop": [mod._dispatcher_entry()]}} + direct = [mod.StopManifest(name="b", path="b", entries=[stop_entry("b")])] + out, changed = mod.mirror(stale, direct) + self.assertTrue(changed) + self.assertEqual(commands(out, "SubagentStop"), ["python3 $HOME/.claude/hooks/b/claude_stop_check.py"]) + def test_empty_result_does_not_leave_an_empty_event_key(self): out, changed = mod.mirror({"hooks": {}}, []) self.assertFalse(changed)