From 217283e2fa7dbb8dbe72afd6b3e29ec038da64aa Mon Sep 17 00:00:00 2001 From: Edbert Chan Date: Fri, 11 Sep 2026 23:21:34 -0700 Subject: [PATCH] feat(ci): fail when a phrase checker has nothing calling it catstack enforces that a HOOK is wired -- test_install.py's TestEveryClaudeHookScriptIsWired caught exactly that on a new hook earlier today. Nothing enforced it for phrase checkers, so a rule could ship as data with no caller, read as covered in review, and never fire. scripts/check_rules_are_wired.py fails unless each checker is either named in quotes by a non-test file, discovered at runtime by a file in its own hook that enumerates the phrases directory, or declares an unwired_reason. That last one mirrors the repo's subagent_stop.inherit:false + reason opt-out: dormant is allowed, silent is not. Wired into CI beside the hook coverage gate. Two false answers were found and fixed while building it, both pinned by tests: - A bare substring match passed `example` off three files that only used the word in prose (scratchpad-collision/detect.py, llm-judge/phrases.py, no-comments/detect.py). The pattern now requires the quoted form; test_an_unquoted_prose_mention_is_not_a_caller holds that. - Requiring the quoted form then failed all five plain-words checkers, which are genuinely wired: diu-stop/plain_words.py loads them by listing the directory, so it names none of them. A gate that only understood literals would have failed the repo's own working code. test_a_hook_that_lists_its_phrases_dir_wires_all_of_them holds that, and test_mentioning_phrases_without_listing_it_is_not_wiring holds the boundary. On the real tree the gate now reports the five as dynamic and flags only llm-judge/phrases/example.json, which has no caller by design and now says so. 18 gate tests, 5 meta-gate tests, preflight green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018u8S5ct3kFhosinSbybc7W Change-Id: Idab82b59bdeef9499f5af5956e23b5be9e32b26b --- .github/workflows/ci.yml | 3 + engine/hooks/llm-judge/phrases/example.json | 3 +- scripts/check_rules_are_wired.py | 213 ++++++++++++++++++++ tests/test_rules_are_wired.py | 125 ++++++++++++ 4 files changed, 343 insertions(+), 1 deletion(-) create mode 100755 scripts/check_rules_are_wired.py create mode 100644 tests/test_rules_are_wired.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7421e8fc..e6e4dfce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,9 @@ jobs: - name: hook e2e coverage gate (positive + negative test per detector) run: python3 scripts/check_hook_test_coverage.py + - name: rule wiring gate (every phrase checker has a caller or a stated reason) + run: python3 scripts/check_rules_are_wired.py + - name: skills three-harness install gate run: python3 scripts/check_skills_three_harnesses.py diff --git a/engine/hooks/llm-judge/phrases/example.json b/engine/hooks/llm-judge/phrases/example.json index c971f088..06019666 100644 --- a/engine/hooks/llm-judge/phrases/example.json +++ b/engine/hooks/llm-judge/phrases/example.json @@ -12,5 +12,6 @@ "You're right. Let's go with option B.", "Nothing in my earlier count was wrong." ], - "on_hit": "example: the last reply took back an earlier claim." + "on_hit": "example: the last reply took back an earlier claim.", + "unwired_reason": "the worked example for llm-judge's README; it documents the phrase-file shape and is loaded only by that hook's own tests, so it must never be submitted against a real reply" } diff --git a/scripts/check_rules_are_wired.py b/scripts/check_rules_are_wired.py new file mode 100755 index 00000000..805fa044 --- /dev/null +++ b/scripts/check_rules_are_wired.py @@ -0,0 +1,213 @@ +#!/usr/bin/env python3 +"""Every phrase checker must be reachable from code, or say why it is not. + +A rule that exists but nothing calls is worse than a missing rule: it reads as +covered in review and never fires in practice. Five `plain-words-*` checkers -- +including the one banning jargon -- shipped with no caller at all, so the rule +was written five ways and enforced zero. + +Wired means one of two things. Either a non-test source file names the checker +in quotes, or a non-test file in the owning hook enumerates that phrases +directory at runtime -- `diu-stop/plain_words.py` loads every `plain-words-*` +file by listing the directory, so it names none of them and wires all of them. +A gate that only understood the quoted form would fail the repo's own working +code. + +The only other acceptable state is an explicit `unwired_reason` in the phrase +file, mirroring this repo's `subagent_stop.inherit: false` + `reason` pattern: +an opt-out has to be stated, not inferred from silence. + +Fail-safe defaults, Saltzer & Schroeder 1975: +https://web.mit.edu/Saltzer/www/publications/protection/Basic.html -- absence of +a caller is a deny, never a pass. +""" +from __future__ import annotations + +import contextlib +import io +import json +import os +import subprocess +import sys +import tempfile + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +HOOKS_ROOT = os.path.join(REPO_ROOT, "engine", "hooks") +SOURCE_SUFFIXES = (".py", ".sh", ".mjs", ".cjs", ".js", ".json") + + +def phrase_files() -> list[str]: + found = [] + for hook in sorted(os.listdir(HOOKS_ROOT)): + phrases_dir = os.path.join(HOOKS_ROOT, hook, "phrases") + if not os.path.isdir(phrases_dir): + continue + for name in sorted(os.listdir(phrases_dir)): + if name.endswith(".json"): + found.append(os.path.join(phrases_dir, name)) + return found + + +def checker_name(path: str) -> str | None: + try: + with open(path, encoding="utf-8") as handle: + data = json.load(handle) + except (OSError, json.JSONDecodeError) as exc: + print(f"check_rules_are_wired: cannot read {path}: {exc}", file=sys.stderr) + return None + name = data.get("checker") + return name if isinstance(name, str) and name else None + + +def unwired_reason(path: str) -> str | None: + try: + with open(path, encoding="utf-8") as handle: + data = json.load(handle) + except (OSError, json.JSONDecodeError): + return None + reason = data.get("unwired_reason") + return reason.strip() if isinstance(reason, str) and reason.strip() else None + + +def callers(name: str) -> list[str]: + """Non-test source files under engine/ that name this checker. + + The name must appear quoted. A bare substring match passes a dormant + checker whose name is an ordinary word: `example` matched three files that + merely used the word in prose. + """ + pattern = f"[\"']{name}[\"']" + try: + result = subprocess.run( + ["grep", "-rlE", "--include=*.py", "--include=*.sh", "--include=*.mjs", + "--include=*.js", "--include=*.json", pattern, HOOKS_ROOT], + capture_output=True, text=True, timeout=60, + ) + except (OSError, subprocess.SubprocessError) as exc: + raise SystemExit(f"check_rules_are_wired: grep failed: {exc}") from exc + + hits = [] + for line in result.stdout.splitlines(): + path = line.strip() + if not path: + continue + parts = path.split(os.sep) + if "phrases" in parts or "tests" in parts or "__pycache__" in parts: + continue + if path.endswith(SOURCE_SUFFIXES): + hits.append(os.path.relpath(path, REPO_ROOT)) + return hits + + +PROMISED_CATCH = ( + "no-caller", + "prose-mention-only", + "empty-unwired-reason", + "no-checker-name", +) +PROMISED_ALLOW = ( + "quoted-caller", + "stated-unwired-reason", + "runtime-discovery", +) + + +def flags_exemplar(exemplar: str) -> bool: + """True when this gate fails on `exemplar`. One throwaway hooks tree each.""" + payload = { + "checker": "demo-rule", + "meaning": "m", + "reads": "reply", + "match": ["a"], + "not_match": ["b"], + "on_hit": "h", + } + if exemplar == "no-checker-name": + payload.pop("checker") + if exemplar == "empty-unwired-reason": + payload["unwired_reason"] = " " + if exemplar == "stated-unwired-reason": + payload["unwired_reason"] = "sample only, never submitted" + + caller = { + "quoted-caller": 'NAMES = ("demo-rule",)\n', + "prose-mention-only": '"""A demo-rule is described here in prose."""\n', + "runtime-discovery": 'import os\nnames = os.listdir("phrases")\n', + }.get(exemplar) + + global HOOKS_ROOT, REPO_ROOT + saved = (HOOKS_ROOT, REPO_ROOT) + with tempfile.TemporaryDirectory() as tmp: + hooks = os.path.join(tmp, "engine", "hooks", "demo") + os.makedirs(os.path.join(hooks, "phrases")) + with open(os.path.join(hooks, "phrases", "demo.json"), "w", encoding="utf-8") as handle: + json.dump(payload, handle) + if caller: + with open(os.path.join(hooks, "detect.py"), "w", encoding="utf-8") as handle: + handle.write(caller) + HOOKS_ROOT = os.path.dirname(hooks) + REPO_ROOT = os.path.dirname(HOOKS_ROOT) + try: + with contextlib.redirect_stdout(io.StringIO()), contextlib.redirect_stderr(io.StringIO()): + return main() != 0 + finally: + HOOKS_ROOT, REPO_ROOT = saved + + +def enumerates_dir(hook_dir: str) -> str | None: + """A non-test file in this hook that lists its own phrases directory.""" + for name in sorted(os.listdir(hook_dir)): + path = os.path.join(hook_dir, name) + if not os.path.isfile(path) or not name.endswith(".py"): + continue + if name.startswith("test"): + continue + try: + with open(path, encoding="utf-8") as handle: + text = handle.read() + except OSError as exc: + print(f"check_rules_are_wired: cannot read {path}: {exc}", file=sys.stderr) + continue + if "phrases" not in text: + continue + if any(call in text for call in ("os.listdir", "glob.glob", ".iterdir(", "scandir")): + return os.path.relpath(path, REPO_ROOT) + return None + + +def main() -> int: + problems = [] + checked = 0 + for path in phrase_files(): + relative = os.path.relpath(path, REPO_ROOT) + name = checker_name(path) + if not name: + problems.append(f"{relative}: no `checker` name, so nothing can call it") + continue + checked += 1 + if callers(name): + continue + dynamic = enumerates_dir(os.path.dirname(os.path.dirname(path))) + if dynamic: + print(f"dynamic {name}: loaded by {dynamic}") + continue + reason = unwired_reason(path) + if reason: + print(f"opt-out {name}: {reason}") + continue + problems.append( + f"{relative}: checker `{name}` has no caller under engine/ outside phrases/ " + "and tests/, and nothing in that hook enumerates the directory. Wire it, or add " + "an `unwired_reason` saying why it ships dormant.") + + if problems: + print("check_rules_are_wired: FAIL", file=sys.stderr) + for problem in problems: + print(f" - {problem}", file=sys.stderr) + return 1 + print(f"check_rules_are_wired: OK ({checked} checker(s) checked)") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_rules_are_wired.py b/tests/test_rules_are_wired.py new file mode 100644 index 00000000..c1ffd667 --- /dev/null +++ b/tests/test_rules_are_wired.py @@ -0,0 +1,125 @@ +#!/usr/bin/env python3 +"""The bad case this gate exists for is real: five `plain-words-*` checkers, +including the jargon ban, shipped with no caller. The false-negative case is +real too -- `example` passed on a bare substring match against three files that +merely used the word in prose. +""" +from __future__ import annotations + +import importlib.util +import json +import os +import subprocess +import sys +import tempfile +import unittest + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +GATE = os.path.join(REPO_ROOT, "scripts", "check_rules_are_wired.py") + + +def load_gate(hooks_root: str): + spec = importlib.util.spec_from_file_location("gate_under_test", GATE) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + module.HOOKS_ROOT = hooks_root + module.REPO_ROOT = os.path.dirname(hooks_root) + return module + + +class WiringGate(unittest.TestCase): + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.hooks = os.path.join(self.tmp.name, "engine", "hooks") + os.makedirs(self.hooks) + + def _phrase(self, hook: str, name: str, **extra) -> str: + directory = os.path.join(self.hooks, hook, "phrases") + os.makedirs(directory, exist_ok=True) + path = os.path.join(directory, f"{name}.json") + payload = { + "checker": name, + "meaning": "whatever", + "reads": "reply", + "match": ["a"], + "not_match": ["b"], + "on_hit": "hit", + } + payload.update(extra) + with open(path, "w", encoding="utf-8") as handle: + json.dump(payload, handle) + return path + + def _caller(self, hook: str, body: str) -> None: + directory = os.path.join(self.hooks, hook) + os.makedirs(directory, exist_ok=True) + with open(os.path.join(directory, "detect.py"), "w", encoding="utf-8") as handle: + handle.write(body) + + def test_a_checker_with_no_caller_fails(self) -> None: + self._phrase("diu-stop", "plain-words-tech-jargon") + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 1) + + def test_a_quoted_caller_passes(self) -> None: + self._phrase("diu-stop", "plain-words-tech-jargon") + self._caller("diu-stop", 'CATEGORIES = ("plain-words-tech-jargon",)\n') + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 0) + + def test_an_unquoted_prose_mention_is_not_a_caller(self) -> None: + self._phrase("llm-judge", "example") + self._caller("llm-judge", '"""For example, this is prose."""\n') + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 1) + + def test_an_explicit_unwired_reason_passes(self) -> None: + self._phrase("llm-judge", "example", unwired_reason="sample only, never submitted") + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 0) + + def test_an_empty_unwired_reason_does_not_count(self) -> None: + self._phrase("llm-judge", "example", unwired_reason=" ") + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 1) + + def test_a_phrase_file_with_no_checker_name_fails(self) -> None: + directory = os.path.join(self.hooks, "diu-stop", "phrases") + os.makedirs(directory) + with open(os.path.join(directory, "broken.json"), "w", encoding="utf-8") as handle: + json.dump({"meaning": "no checker key"}, handle) + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 1) + + def test_a_caller_inside_tests_does_not_count(self) -> None: + self._phrase("diu-stop", "plain-words-tech-jargon") + tests_dir = os.path.join(self.hooks, "diu-stop", "tests") + os.makedirs(tests_dir) + with open(os.path.join(tests_dir, "test_x.py"), "w", encoding="utf-8") as handle: + handle.write('load("plain-words-tech-jargon")\n') + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 1) + + def test_the_real_repo_passes(self) -> None: + result = subprocess.run([sys.executable, GATE], capture_output=True, text=True, timeout=120) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + +class RuntimeDiscovery(WiringGate): + def test_a_hook_that_lists_its_phrases_dir_wires_all_of_them(self) -> None: + self._phrase("diu-stop", "plain-words-tech-jargon") + self._phrase("diu-stop", "plain-words-code-names") + self._caller("diu-stop", 'import os\nnames = os.listdir("phrases")\n') + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 0) + + def test_mentioning_phrases_without_listing_it_is_not_wiring(self) -> None: + self._phrase("diu-stop", "plain-words-tech-jargon") + self._caller("diu-stop", '"""The phrases directory holds the word lists."""\n') + gate = load_gate(self.hooks) + self.assertEqual(gate.main(), 1) + + +if __name__ == "__main__": + unittest.main()