From f61550d1f7c07d917bdc8ecb0e2b703f88753519 Mon Sep 17 00:00:00 2001 From: Edbert Chan Date: Sat, 12 Sep 2026 19:59:57 -0700 Subject: [PATCH] feat(ci): fail a hook that imports the judge when its tests skip the base check_hook_test_coverage now checks every hook folder, not only ones with detect.py, so diu-stop and llm-judge are covered. It reads imports and class bases with ast instead of a substring match, and reports a file it could not parse as unchecked rather than clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013smxPCr4XeE7R77zVtN1u4 Change-Id: I09dbaf2ffd8b6c394201c530dce90516517ece05 --- scripts/check_hook_test_coverage.py | 77 +++++++++++++++++++++++++- tests/test_check_hook_test_coverage.py | 49 ++++++++++++++++ 2 files changed, 123 insertions(+), 3 deletions(-) diff --git a/scripts/check_hook_test_coverage.py b/scripts/check_hook_test_coverage.py index 6918d931..77f96a59 100755 --- a/scripts/check_hook_test_coverage.py +++ b/scripts/check_hook_test_coverage.py @@ -161,6 +161,76 @@ def hooks_with_detector() -> list[str]: return found +def _parse_python_files(folder: str, skip_tests: bool) -> tuple[list[ast.AST], list[str]]: + trees: list[ast.AST] = [] + unreadable: list[str] = [] + for root, dirs, files in os.walk(folder): + if skip_tests: + dirs[:] = [directory for directory in dirs if directory != "tests"] + for filename in sorted(files): + if not filename.endswith(".py"): + continue + path = os.path.join(root, filename) + try: + with open(path, encoding="utf-8") as handle: + trees.append(ast.parse(handle.read(), filename=path)) + except (OSError, SyntaxError, UnicodeDecodeError) as error: + unreadable.append(f"{path} ({type(error).__name__}: {error})") + return trees, unreadable + + +def _imports_judge(tree: ast.AST) -> bool: + for node in ast.walk(tree): + if isinstance(node, ast.Import) and any( + alias.name == "judge" or alias.name.endswith(".judge") for alias in node.names + ): + return True + if isinstance(node, ast.ImportFrom) and node.module and ( + node.module == "judge" or node.module.endswith(".judge") + ): + return True + return False + + +def _uses_judge_test_base(tree: ast.AST) -> bool: + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and any(alias.name == "JudgeTestCase" for alias in node.names): + return True + if isinstance(node, ast.ClassDef) and any( + (isinstance(base, ast.Name) and base.id == "JudgeTestCase") + or (isinstance(base, ast.Attribute) and base.attr == "JudgeTestCase") + for base in node.bases + ): + return True + return False + + +def judge_isolation_problems(hook_dir: str) -> list[str]: + name = os.path.basename(hook_dir) + sources, unreadable = _parse_python_files(hook_dir, skip_tests=True) + problems = [f"{name}: could not read {path}, so its judge imports are unchecked" for path in unreadable] + if not any(_imports_judge(tree) for tree in sources): + return problems + tests_dir = os.path.join(hook_dir, "tests") + if not os.path.isdir(tests_dir): + return problems + [f"{name}: imports llm-judge but has no tests/ dir using JudgeTestCase"] + tests, unreadable_tests = _parse_python_files(tests_dir, skip_tests=False) + problems += [f"{name}: could not read {path}, so its JudgeTestCase use is unchecked" for path in unreadable_tests] + if not any(_uses_judge_test_base(tree) for tree in tests): + problems.append(f"{name}: imports llm-judge but no test imports or subclasses JudgeTestCase") + return problems + + +def hook_dirs() -> list[str]: + if not os.path.isdir(HOOKS_DIR): + raise FileNotFoundError(f"hooks folder not found, so no hook's judge isolation was checked: {HOOKS_DIR}") + return [ + os.path.join(HOOKS_DIR, name) + for name in sorted(os.listdir(HOOKS_DIR)) + if os.path.isdir(os.path.join(HOOKS_DIR, name)) + ] + + def check_hook(hook_dir: str) -> list[str]: """Return a list of problems for this hook, empty if it passes.""" name = os.path.basename(hook_dir) @@ -203,9 +273,10 @@ def main() -> int: all_problems: list[str] = [] for hook_dir in targets: - if not os.path.isfile(os.path.join(hook_dir, "detect.py")): - continue - all_problems.extend(check_hook(hook_dir)) + if os.path.isfile(os.path.join(hook_dir, "detect.py")): + all_problems.extend(check_hook(hook_dir)) + for hook_dir in [os.path.abspath(a) for a in args] if args else hook_dirs(): + all_problems.extend(judge_isolation_problems(hook_dir)) if all_problems: print("check_hook_test_coverage: FAIL") diff --git a/tests/test_check_hook_test_coverage.py b/tests/test_check_hook_test_coverage.py index 13a05d95..cf83966a 100644 --- a/tests/test_check_hook_test_coverage.py +++ b/tests/test_check_hook_test_coverage.py @@ -12,6 +12,7 @@ import tempfile import unittest from pathlib import Path +from unittest.mock import patch REPO = Path(__file__).resolve().parents[1] sys.path.insert(0, str(REPO / "scripts")) @@ -97,3 +98,51 @@ def test_the_real_repo_passes_this_gate(self): if __name__ == "__main__": unittest.main() + + +JUDGE_CALLER = "def ask(text):\n import judge\n return judge.ask(text)\n" +USES_BASE = "from testing import JudgeTestCase\n\n\nclass TestCaller(JudgeTestCase):\n pass\n" +NAMES_BASE_IN_COMMENT = "import unittest\n\n\nclass TestCaller(unittest.TestCase):\n pass # JudgeTestCase\n" + + +def judge_hook(root: Path, source: str, tests: str | None) -> Path: + hook_dir = root / "caller" + hook_dir.mkdir() + (hook_dir / "caller.py").write_text(source, encoding="utf-8") + if tests is not None: + (hook_dir / "tests").mkdir() + (hook_dir / "tests" / "test_caller.py").write_text(tests, encoding="utf-8") + return hook_dir + + +class TestJudgeIsolationRule(unittest.TestCase): + def test_hit_judge_caller_whose_tests_skip_the_base_fails(self): + with tempfile.TemporaryDirectory() as tmp: + problems = chtc.judge_isolation_problems(str(judge_hook(Path(tmp), JUDGE_CALLER, FIRES_AND_SILENT))) + self.assertEqual(len(problems), 1, problems) + self.assertIn("JudgeTestCase", problems[0]) + + def test_hit_base_named_only_in_a_comment_still_fails(self): + with tempfile.TemporaryDirectory() as tmp: + problems = chtc.judge_isolation_problems(str(judge_hook(Path(tmp), JUDGE_CALLER, NAMES_BASE_IN_COMMENT))) + self.assertEqual(len(problems), 1, problems) + + def test_no_hit_judge_caller_whose_tests_subclass_the_base_passes(self): + with tempfile.TemporaryDirectory() as tmp: + self.assertEqual(chtc.judge_isolation_problems(str(judge_hook(Path(tmp), JUDGE_CALLER, USES_BASE))), []) + + def test_no_hit_hook_that_never_imports_judge_is_not_asked_for_the_base(self): + with tempfile.TemporaryDirectory() as tmp: + self.assertEqual(chtc.judge_isolation_problems(str(judge_hook(Path(tmp), INLINE_DETECTOR, FIRES_AND_SILENT))), []) + + def test_unreadable_source_is_reported_as_unchecked_not_clean(self): + with tempfile.TemporaryDirectory() as tmp: + problems = chtc.judge_isolation_problems(str(judge_hook(Path(tmp), "def broken(:\n", USES_BASE))) + self.assertEqual(len(problems), 1, problems) + self.assertIn("could not read", problems[0]) + + def test_hit_hook_without_detect_py_is_still_checked_by_main(self): + with tempfile.TemporaryDirectory() as tmp: + hook_dir = judge_hook(Path(tmp), JUDGE_CALLER, FIRES_AND_SILENT) + with patch.object(sys, "argv", ["check_hook_test_coverage.py", str(hook_dir)]): + self.assertEqual(chtc.main(), 1)