From 17a2b07cb40e9e52106c37d4f713dad7815b2f66 Mon Sep 17 00:00:00 2001 From: Edbert Chan Date: Sat, 12 Sep 2026 20:01:52 -0700 Subject: [PATCH] [Hooks Cannot See] (1) Teach two hooks the checks they could not see narrow-the-scope fired four times in one session and was wrong all four. Its VERIFY_RE knew pytest, npm test, tsc and friends, but nothing about `bash scripts/test-foo.sh` or `docker build` -- so a shell-and-container project looks exactly like an edit streak with no verification in it, and the reminder becomes noise. A detector that cries wolf trains the reader to skip it, so this adds those patterns plus one more reset rule: a Bash command naming the basename of a file in the current streak clears that file, because running the script you just edited is how this kind of work gets checked and no regex can enumerate every project's entry point. tests/fixtures/shell_verified_streak_2026-09-11.json is the verbatim sequence from that session, silent with the verification row and firing without it. hook-freshness watched the checkout behind ~/.claude/hooks for staleness but never checked that the registered hook scripts still resolve. In that same session ~/.claude/hooks/split-scope pointed into a deleted worktree: the hook was registered, could not run, and said nothing for the whole session, which is "a check that could not run is not a pass" breaking on the gates themselves. The sweep reads settings.json, expands the paths, and names every one that does not resolve. It returns three outcomes, not two -- an unreadable or unparseable settings.json reports unchecked rather than clean, which the first draft of this change got wrong until explicit-failures caught it. install.sh warns when it is linking out of a git worktree, so the next dead link is announced as it is created rather than found a session later. It deliberately does not redirect to the main checkout: tests/test_install.py:290 specifies that install.sh links against its own checkout, because installing from a worktree is how a branch gets tested. An earlier draft did redirect, and that test caught it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011ZqCxSQVpmM9Rx37C7irgf Change-Id: I09fda6fc2dd315cd16eca766d881cb6f2b7306e6 --- engine/hooks/hook-freshness/detect.py | 96 ++++++++++++++++++- .../hooks/hook-freshness/tests/test_hooks.py | 71 ++++++++++++++ engine/hooks/narrow-the-scope/README.md | 17 +++- engine/hooks/narrow-the-scope/detect.py | 19 +++- .../shell_verified_streak_2026-09-11.json | 26 +++++ .../narrow-the-scope/tests/test_hooks.py | 43 +++++++++ install.sh | 23 +++++ tests/test_install_worktree_repo_dir.py | 90 +++++++++++++++++ 8 files changed, 377 insertions(+), 8 deletions(-) create mode 100644 engine/hooks/narrow-the-scope/tests/fixtures/shell_verified_streak_2026-09-11.json create mode 100644 tests/test_install_worktree_repo_dir.py diff --git a/engine/hooks/hook-freshness/detect.py b/engine/hooks/hook-freshness/detect.py index 6cb7dfa2..d6d400a5 100644 --- a/engine/hooks/hook-freshness/detect.py +++ b/engine/hooks/hook-freshness/detect.py @@ -96,6 +96,88 @@ def advisory(repo, branch, behind): return MESSAGE.format(detail=" and ".join(parts), repo=repo, trunk=TRUNK) +SETTINGS_PATH = os.path.join(os.path.expanduser("~"), ".claude", "settings.json") + +UNCHECKED_MESSAGE = ( + "hook-freshness: could not check whether the registered hook scripts resolve, because " + "{reason}. Treat the hook set as unchecked rather than healthy." +) + +UNRESOLVABLE_MESSAGE = ( + "hook-freshness: {count} registered hook script(s) cannot run because their path does " + "not resolve: {paths}. Those hooks are unchecked, not clean — a gate that never " + "executes reports nothing. Re-run your catstack `install.sh` to relink them." +) + + +def _load_json(path): + with open(path, encoding="utf-8") as handle: + return json.load(handle) + + +def _hook_commands(settings_path=SETTINGS_PATH, load=None): + """(commands, unreadable_reason). A reason means the sweep could not run at all.""" + loader = load or _load_json + try: + data = loader(settings_path) + except FileNotFoundError: + return [], f"{settings_path} does not exist" + except (OSError, ValueError) as exc: + return [], f"{settings_path} could not be read ({type(exc).__name__}: {exc})" + hooks = data.get("hooks") + if not isinstance(hooks, dict): + return [], f"{settings_path} has no readable 'hooks' object" + commands = [] + malformed = 0 + for matchers in hooks.values(): + if not isinstance(matchers, list): + malformed += 1 + continue + for matcher in matchers: + if not isinstance(matcher, dict): + malformed += 1 + continue + for entry in matcher.get("hooks") or []: + if isinstance(entry, dict) and entry.get("command"): + commands.append(str(entry["command"])) + else: + malformed += 1 + if malformed and not commands: + return [], f"{settings_path} has {malformed} hook entr(ies) in an unrecognised shape and no readable command" + return commands, None + + +def _script_paths(command): + expanded = os.path.expandvars(command).replace("~/", os.path.expanduser("~") + "/") + return [tok for tok in expanded.split() if "/" in tok and not tok.startswith("-")] + + +def unresolvable_hooks(settings_path=SETTINGS_PATH, load=None, exists=os.path.exists): + """(missing script paths, unreadable_reason). Never reports clean when it could not look.""" + commands, unreadable = _hook_commands(settings_path, load=load) + if unreadable: + return [], unreadable + missing = [] + for command in commands: + for path in _script_paths(command): + if "$" in path: + continue + if not exists(path) and path not in missing: + missing.append(path) + return missing, None + + +def unresolvable_advisory(missing, unreadable=None): + if unreadable: + return UNCHECKED_MESSAGE.format(reason=unreadable) + if not missing: + return None + shown = ", ".join(missing[:3]) + if len(missing) > 3: + shown += f", and {len(missing) - 3} more" + return UNRESOLVABLE_MESSAGE.format(count=len(missing), paths=shown) + + def _state_file(key): digest = hashlib.sha256((key or "no-transcript").encode("utf-8")).hexdigest()[:16] return os.path.join(STATE_DIR, f"{digest}.advised") @@ -123,12 +205,18 @@ def decide(payload, env=None, run=_run_git, state=True): key = payload.get("transcript_path") or payload.get("transcriptPath") or "" if state and already_advised(key): return None + missing, unreadable = unresolvable_hooks() + lines = [ln for ln in [unresolvable_advisory(missing, unreadable)] if ln] repo = resolve_repo(env=env) - if not repo: + if repo: + branch, behind = repo_state(repo, env=env, run=run) + staleness = advisory(repo, branch, behind) + if staleness: + lines.append(staleness) + if not lines: return None - branch, behind = repo_state(repo, env=env, run=run) - line = advisory(repo, branch, behind) - if line and state: + line = "\n".join(lines) + if state: mark_advised(key) return line diff --git a/engine/hooks/hook-freshness/tests/test_hooks.py b/engine/hooks/hook-freshness/tests/test_hooks.py index 38f91a92..006ff874 100644 --- a/engine/hooks/hook-freshness/tests/test_hooks.py +++ b/engine/hooks/hook-freshness/tests/test_hooks.py @@ -142,3 +142,74 @@ def test_hit_resolves_repo_from_symlink_target(self): if __name__ == "__main__": unittest.main() + + +class TestUnresolvableHookSweep(unittest.TestCase): + """A registered hook whose script path is gone is unchecked, never clean. + + Mirrors the real failure: ~/.claude/hooks/split-scope pointed into a deleted + worktree, so the gate could not run for a whole session and said nothing. + """ + + def _settings(self, commands): + return {"hooks": {"UserPromptSubmit": [{"matcher": "*", "hooks": [ + {"type": "command", "command": c} for c in commands + ]}]}} + + def test_names_a_registered_hook_whose_script_is_missing(self): + settings = self._settings([ + "python3 /real/hooks/diu-stop/claude_stop_check.py", + "python3 /gone/hooks/split-scope/claude_prompt_submit.py", + ]) + missing, unreadable = detect.unresolvable_hooks( + settings_path="/tmp/settings.json", + load=lambda _p: settings, + exists=lambda p: p.startswith("/real/"), + ) + self.assertIsNone(unreadable) + self.assertEqual(missing, ["/gone/hooks/split-scope/claude_prompt_submit.py"]) + line = detect.unresolvable_advisory(missing, unreadable) + self.assertIn("split-scope", line) + self.assertIn("unchecked, not clean", line) + + def test_silent_when_every_registered_hook_resolves(self): + settings = self._settings(["python3 /real/hooks/diu-stop/claude_stop_check.py"]) + missing, unreadable = detect.unresolvable_hooks( + settings_path="/tmp/settings.json", + load=lambda _p: settings, + exists=lambda _p: True, + ) + self.assertEqual((missing, unreadable), ([], None)) + self.assertIsNone(detect.unresolvable_advisory(missing, unreadable)) + + def test_an_unreadable_settings_file_reports_unchecked_not_clean(self): + def boom(_path): + raise ValueError("Expecting ',' delimiter: line 4 column 3") + + missing, unreadable = detect.unresolvable_hooks( + settings_path="/tmp/settings.json", load=boom, exists=lambda _p: True, + ) + self.assertEqual(missing, []) + self.assertIsNotNone(unreadable) + line = detect.unresolvable_advisory(missing, unreadable) + self.assertIn("could not check", line) + self.assertIn("unchecked rather than healthy", line) + + def test_a_missing_settings_file_reports_unchecked_not_clean(self): + def gone(_path): + raise FileNotFoundError(2, "No such file or directory") + + missing, unreadable = detect.unresolvable_hooks( + settings_path="/tmp/settings.json", load=gone, exists=lambda _p: True, + ) + self.assertEqual(missing, []) + self.assertIn("does not exist", unreadable) + + def test_unexpanded_variables_are_not_reported_as_missing(self): + settings = self._settings(["python3 $UNSET_ROOT/hooks/x/run.py"]) + missing, unreadable = detect.unresolvable_hooks( + settings_path="/tmp/settings.json", + load=lambda _p: settings, + exists=lambda _p: False, + ) + self.assertEqual((missing, unreadable), ([], None)) diff --git a/engine/hooks/narrow-the-scope/README.md b/engine/hooks/narrow-the-scope/README.md index 41127a9d..711f1776 100644 --- a/engine/hooks/narrow-the-scope/README.md +++ b/engine/hooks/narrow-the-scope/README.md @@ -3,9 +3,20 @@ PostToolUse (Edit|Write|MultiEdit|NotebookEdit|StrReplace|Bash): count edits per file in session state; a verification-shaped Bash command (pytest, unittest, npm test, jest, vitest, tsc, eslint, ruff, mypy, `check_*.py`, -`run_all_tests`, cargo/go test, ...) resets every count. When a file reaches -three edits with no reset, inject the `narrow-the-scope` reminder once for -that streak episode. Inject-only, never blocks, fail-open. +`run_all_tests`, cargo/go test, `bash test.sh`, `docker build`, ...) +resets every count. When a file reaches three edits with no reset, inject the +`narrow-the-scope` reminder once for that streak episode. Inject-only, never +blocks, fail-open. + +A Bash command that names the basename of a file in the current streak also +clears that one file's count: running the script you just edited is how shell +and container work gets verified, and `VERIFY_RE` cannot enumerate every +project's entry point. Precision matters more than recall here — a detector +that cries wolf trains the reader to skip it (Kim & Ernst, "Which warnings +should I fix first?", ESEC/FSE 2007, +https://dl.acm.org/doi/10.1145/1287624.1287633). `tests/fixtures/shell_verified_streak_2026-09-11.json` +is the verbatim sequence from a session where this hook fired four times and +was wrong all four. Mechanical half of `product/skills/narrow-the-scope`, whose trigger text is "three or more edits to the same file without a passing test/build/lint run diff --git a/engine/hooks/narrow-the-scope/detect.py b/engine/hooks/narrow-the-scope/detect.py index 9c4f5a80..0e7a49d4 100644 --- a/engine/hooks/narrow-the-scope/detect.py +++ b/engine/hooks/narrow-the-scope/detect.py @@ -26,11 +26,18 @@ r"\b(?:pytest|unittest|npm (?:run )?test|pnpm (?:run )?test|yarn test|jest|vitest|" r"cargo (?:test|check|build)|go (?:test|build|vet)|make (?:test|check|lint)|tsc\b|eslint|" r"ruff|mypy|pyright|flake8|run_all_tests|check_\w+\.py|gradle(?:w)? (?:test|build)|" - r"swift (?:test|build)|xcodebuild|dotnet test|python3? -m (?:pytest|unittest)|node --test)", + r"swift (?:test|build)|xcodebuild|dotnet test|python3? -m (?:pytest|unittest)|node --test|" + r"(?:ba)?sh\s+\S*(?:test|check|verify|prove|run)[\w.-]*\.(?:sh|bash)|" + r"docker\s+(?:build|compose\s+(?:build|up))|" + r"node --check)", re.IGNORECASE, ) +def _basename(path: str) -> str: + return path.rsplit("/", 1)[-1] + + def _file_of(payload: dict) -> str: inp = payload.get("tool_input") or {} return str(inp.get("file_path") or inp.get("notebook_path") or inp.get("path") or "") @@ -56,6 +63,16 @@ def observe(payload: dict) -> str | None: state["counts"] = {} state["fired"] = [] save_state(payload, state) + return None + executed = [p for p in counts if _basename(p) and _basename(p) in cmd] + if executed: + for path in executed: + counts.pop(path, None) + if path in fired: + fired.remove(path) + state["counts"] = counts + state["fired"] = fired + save_state(payload, state) return None if tool not in EDIT_TOOLS: return None diff --git a/engine/hooks/narrow-the-scope/tests/fixtures/shell_verified_streak_2026-09-11.json b/engine/hooks/narrow-the-scope/tests/fixtures/shell_verified_streak_2026-09-11.json new file mode 100644 index 00000000..2c7461db --- /dev/null +++ b/engine/hooks/narrow-the-scope/tests/fixtures/shell_verified_streak_2026-09-11.json @@ -0,0 +1,26 @@ +{ + "source": "Claude Code session c2a4bea7-2e0b-428f-9dbb-0498a9daedeb, building scripts/e2e-cli-install for Invoker", + "why": "The detector fired four times in this session and every one was a false positive: the verification between the edits was a bash script run and a docker build, neither of which VERIFY_RE could see. This is the sandbox-guard slice of that sequence, verbatim.", + "verification_index": 2, + "sequence": [ + {"tool": "Write", "file_path": "/w/scripts/e2e-cli-install/lib/sandbox-guard.sh"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/lib/sandbox-guard.sh"}, + {"tool": "Bash", "command": "bash \"$WT/scripts/test-e2e-cli-install-guard.sh\""}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/lib/sandbox-guard.sh"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/lib/sandbox-guard.sh"} + ], + "docker_sequence": [ + {"tool": "Write", "file_path": "/w/scripts/e2e-cli-install/Dockerfile"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/Dockerfile"}, + {"tool": "Bash", "command": "bash scripts/e2e-cli-install/run.sh --docker"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/Dockerfile"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/Dockerfile"} + ], + "direct_execution_sequence": [ + {"tool": "Write", "file_path": "/w/scripts/e2e-cli-install/run.sh"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/run.sh"}, + {"tool": "Bash", "command": "bash /w/scripts/e2e-cli-install/run.sh --docker"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/run.sh"}, + {"tool": "Edit", "file_path": "/w/scripts/e2e-cli-install/run.sh"} + ] +} diff --git a/engine/hooks/narrow-the-scope/tests/test_hooks.py b/engine/hooks/narrow-the-scope/tests/test_hooks.py index 41c6a213..2e3f30ce 100644 --- a/engine/hooks/narrow-the-scope/tests/test_hooks.py +++ b/engine/hooks/narrow-the-scope/tests/test_hooks.py @@ -108,3 +108,46 @@ def test_fails_open_on_garbage_stdin(self): if __name__ == "__main__": unittest.main() + + +SHELL_FIXTURE = os.path.join( + os.path.dirname(os.path.abspath(__file__)), "fixtures", "shell_verified_streak_2026-09-11.json" +) + + +def load_shell_fixture(): + with open(SHELL_FIXTURE, encoding="utf-8") as fh: + return json.load(fh) + + +class TestShellAndContainerVerificationCounts(_Base): + def _fire_indices(self, sequence, session): + return [i for i, s in enumerate(sequence, 1) if self.detect.observe(payload_for(s, session))] + + def test_silent_when_a_bash_script_run_sits_between_the_edits(self): + fx = load_shell_fixture() + self.assertEqual(self._fire_indices(fx["sequence"], "shell-ok"), []) + + def test_fires_when_that_same_bash_script_run_is_removed(self): + fx = load_shell_fixture() + seq = [s for i, s in enumerate(fx["sequence"]) if i != fx["verification_index"]] + self.assertEqual(self._fire_indices(seq, "shell-missing"), [3]) + + def test_silent_when_a_docker_build_sits_between_the_edits(self): + fx = load_shell_fixture() + self.assertEqual(self._fire_indices(fx["docker_sequence"], "docker-ok"), []) + + def test_fires_when_that_same_docker_build_is_removed(self): + fx = load_shell_fixture() + seq = [s for i, s in enumerate(fx["docker_sequence"]) if i != fx["verification_index"]] + self.assertEqual(self._fire_indices(seq, "docker-missing"), [3]) + + def test_running_the_edited_file_itself_counts_as_verification(self): + fx = load_shell_fixture() + self.assertEqual(self._fire_indices(fx["direct_execution_sequence"], "direct-ok"), []) + + def test_an_unrelated_bash_command_does_not_reset_the_streak(self): + edit = {"session_id": "unrelated", "tool_name": "Edit", "tool_input": {"file_path": "/w/a.sh"}} + noise = {"session_id": "unrelated", "tool_name": "Bash", "tool_input": {"command": "git status --porcelain"}} + seq = [edit, edit, noise, edit] + self.assertEqual([i for i, p in enumerate(seq, 1) if self.detect.observe(p)], [4]) diff --git a/install.sh b/install.sh index dfdd3347..5a32177d 100755 --- a/install.sh +++ b/install.sh @@ -10,6 +10,29 @@ set -euo pipefail REPO_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +resolve_main_checkout() { + local start="$1" common parent + common="$(git -C "$start" rev-parse --path-format=absolute --git-common-dir 2>/dev/null)" || return 1 + [ -n "$common" ] || return 1 + parent="$(cd "$(dirname "$common")" 2>/dev/null && pwd -P)" || return 1 + [ -f "$parent/install.sh" ] || return 1 + printf '%s' "$parent" +} + +warn_if_installing_from_worktree() { + local main_checkout + main_checkout="$(resolve_main_checkout "$REPO_DIR")" || return 0 + [ "$main_checkout" != "$(cd "$REPO_DIR" && pwd -P)" ] || return 0 + echo "install.sh: WARNING — installing from a git worktree, not the main checkout." + echo " worktree: $REPO_DIR" + echo " main checkout: $main_checkout" + echo " Every link below points into the worktree and dies when the worktree is removed," + echo " leaving those hooks registered but unrunnable. That is intended while you test a" + echo " branch; re-run $main_checkout/install.sh when you are done." +} + +warn_if_installing_from_worktree + if [ -z "${CAT_MODE_AUTO_INVOKE:-}" ] && [ -f "$REPO_DIR/.env" ]; then CAT_MODE_AUTO_INVOKE="$(grep -m1 '^CAT_MODE_AUTO_INVOKE=' "$REPO_DIR/.env" | cut -d= -f2-)" fi diff --git a/tests/test_install_worktree_repo_dir.py b/tests/test_install_worktree_repo_dir.py new file mode 100644 index 00000000..3a107859 --- /dev/null +++ b/tests/test_install_worktree_repo_dir.py @@ -0,0 +1,90 @@ +#!/usr/bin/env python3 +"""install.sh warns, loudly, when it is linking out of a git worktree. + +Real failure this pins: ~/.claude/hooks/split-scope pointed at +catstack-wt-pr450-fix/engine/hooks/split-scope. That worktree was deleted, so +the hook could not run for an entire session and reported nothing at all. + +Linking against its own checkout is deliberate (see install.sh's header and +tests/test_install.py), because installing from a worktree is how a branch gets +tested. So this asserts the warning, never a redirect; hook-freshness's +unresolvable-hook sweep is what catches the link after the worktree is gone. + +The fixture is a synthetic repo holding a copy of the working-tree install.sh, +so the test exercises the file as it stands now rather than whatever is at HEAD, +and passes whether the suite itself runs from the main checkout or a worktree. + +Run: python3 -m unittest tests.test_install_worktree_repo_dir -v +""" +from __future__ import annotations + +import os +import shutil +import subprocess +import tempfile +import unittest + +REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +INSTALL = os.path.join(REPO, "install.sh") +NOTICE = "installing from a git worktree" + + +def git(args, cwd, timeout=60): + return subprocess.run( + ["git", *args], cwd=cwd, capture_output=True, text=True, check=False, timeout=timeout, + ) + + +def run_help(script): + return subprocess.run( + ["bash", script, "--help"], capture_output=True, text=True, check=False, timeout=60, + ) + + +class TestInstallResolvesMainCheckout(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix="catstack-install-worktree-") + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + self.main = os.path.join(self.tmp, "main") + os.makedirs(self.main) + shutil.copy2(INSTALL, os.path.join(self.main, "install.sh")) + for args in ( + ["init", "--quiet", "-b", "main"], + ["config", "user.email", "probe@example.invalid"], + ["config", "user.name", "probe"], + ["add", "install.sh"], + ["commit", "--quiet", "-m", "probe"], + ): + result = git(args, self.main) + if result.returncode != 0: + self.skipTest(f"could not build the probe repo: git {args[0]}: {result.stderr.strip()}") + self.main_real = os.path.realpath(self.main) + + def test_the_main_checkout_does_not_warn(self): + result = run_help(os.path.join(self.main, "install.sh")) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertNotIn(NOTICE, result.stdout) + + def test_a_worktree_warns_and_names_the_main_checkout(self): + wt = os.path.join(self.tmp, "wt") + added = git(["worktree", "add", "--quiet", "--detach", wt, "HEAD"], self.main) + if added.returncode != 0: + self.skipTest(f"could not create a probe worktree: {added.stderr.strip()}") + result = run_help(os.path.join(wt, "install.sh")) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(NOTICE, result.stdout) + self.assertIn(f"main checkout: {self.main_real}", result.stdout) + self.assertIn(f"worktree: {wt}", result.stdout) + self.assertIn("dies when the worktree is removed", result.stdout) + + def test_outside_any_git_repo_it_stays_quiet(self): + loose = os.path.join(self.tmp, "loose") + os.makedirs(loose) + shutil.copy2(INSTALL, os.path.join(loose, "install.sh")) + result = run_help(os.path.join(loose, "install.sh")) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertNotIn(NOTICE, result.stdout) + + +if __name__ == "__main__": + unittest.main()