From ac4631a5b0a1cacd2838bd9e6d89c5178420e0e7 Mon Sep 17 00:00:00 2001 From: Allan Date: Sun, 27 Sep 2026 11:31:31 -0700 Subject: [PATCH] fix: sync template hook fix for compound commits and visible warnings Port i-machine-things/.claude#16 (plus the earlier tool_input fix where it was still missing): - read the command from tool_input.command, not the top-level key - inspect every simple command, so `git add f && git commit` is caught - print warnings as PreToolUse JSON (systemMessage + additionalContext) so they are actually shown, not just logged - the shared broad-except check now only looks at .py files This repo's own checks are unchanged. Adds the template hook tests. Co-Authored-By: Claude Opus 5.5 --- .claude/hooks/pre_commit_sp_check.py | 96 +++++++++++++++++------ .claude/hooks/test_pre_commit_sp_check.py | 83 ++++++++++++++++++++ 2 files changed, 154 insertions(+), 25 deletions(-) create mode 100644 .claude/hooks/test_pre_commit_sp_check.py diff --git a/.claude/hooks/pre_commit_sp_check.py b/.claude/hooks/pre_commit_sp_check.py index 731fa6d..091da15 100644 --- a/.claude/hooks/pre_commit_sp_check.py +++ b/.claude/hooks/pre_commit_sp_check.py @@ -13,24 +13,40 @@ import subprocess -def _is_git_commit_command(command: str) -> bool: - """Return True only when command invokes the git commit subcommand.""" - try: - tokens = shlex.split(command, posix=True) - except ValueError: - return bool(re.search(r"git\s+commit", command)) +# git global options that consume the following token as their argument +_ARG_OPTIONS = frozenset({ + "-C", "-c", "--exec-path", "--git-dir", + "--work-tree", "--namespace", "--super-prefix", + "--list-cmds", +}) +_OPERATOR_CHARS = frozenset("();<>|&") + + +def _command_segments(command: str) -> list: + """Split a shell command line into simple commands at &&, ||, ;, |, & and newlines. + + Only the first git invocation used to be inspected, so `git add f && git commit` looked like a plain + `git add` and the staged-diff check silently never ran. Newlines are turned into `;` first because shlex + treats them as plain whitespace; inside quotes (a multi-line commit message) they stay part of one token. + """ + lexer = shlex.shlex(command.replace("\n", " ; "), posix=True, punctuation_chars=True) + lexer.whitespace_split = True + segments, current = [], [] + for tok in lexer: + if tok and all(ch in _OPERATOR_CHARS for ch in tok): + segments.append(current) + current = [] + else: + current.append(tok) + segments.append(current) + return [seg for seg in segments if seg] + +def _segment_is_git_commit(tokens: list) -> bool: try: git_idx = next(i for i, t in enumerate(tokens) if t == "git" or t.endswith("/git")) except StopIteration: return False - - # Options that consume the following token as an argument - _ARG_OPTIONS = frozenset({ - "-C", "-c", "--exec-path", "--git-dir", - "--work-tree", "--namespace", "--super-prefix", - "--list-cmds", - }) i = git_idx + 1 while i < len(tokens): tok = tokens[i] @@ -43,6 +59,26 @@ def _is_git_commit_command(command: str) -> bool: return False +def _is_git_commit_command(command: str) -> bool: + """Return True when any simple command in the line invokes the git commit subcommand.""" + try: + segments = _command_segments(command) + except ValueError: + return bool(re.search(r"git\s+commit", command)) + return any(_segment_is_git_commit(seg) for seg in segments) + + +def _added_lines_by_file(diff: str) -> list: + """Return (path, added line) pairs from a unified diff, so checks can be limited to the right file types.""" + pairs, path = [], "" + for line in diff.splitlines(): + if line.startswith("+++ "): + path = line[4:].removeprefix("b/") + elif line.startswith("+"): + pairs.append((path, line)) + return pairs + + def get_staged_diff() -> str: git = shutil.which("git") if not git: @@ -60,7 +96,11 @@ def get_staged_diff() -> str: def main(): try: data = json.load(sys.stdin) - command = data.get("command", "") + # A PreToolUse hook matched on Bash receives the command nested at tool_input.command, not + # top-level — reading the top-level key always returned "", so this check silently no-op'd + # on every single commit since it was written. Confirmed against Claude Code's real + # PreToolUse hook input schema and caught by CodeRabbit on i-machine-arr/reaparr PR #2. + command = data.get("tool_input", {}).get("command", "") except json.JSONDecodeError: sys.exit(0) @@ -71,7 +111,8 @@ def main(): if not diff: sys.exit(0) - added_lines = [line for line in diff.splitlines() if line.startswith("+") and not line.startswith("+++")] + added = _added_lines_by_file(diff) + added_lines = [line for _, line in added] # line-only view used by the checks below warnings = [] @@ -80,8 +121,9 @@ def main(): # --------------------------------------------------------------- # Broad except Exception - for line in added_lines: - if re.search(r"except\s+Exception\b", line): + # Python files only: docs that show the anti-pattern as an example must not trip it. + for path, line in added: + if path.endswith(".py") and re.search(r"except\s+Exception\b", line): warnings.append( "Broad 'except Exception' detected — use specific exceptions " "(e.g. OSError, AttributeError)." @@ -100,14 +142,18 @@ def main(): # --------------------------------------------------------------- if warnings: - print() - print(f"Coding Notes Pre-commit Check — {len(warnings)} issue(s) found:") - for w in warnings: - print(f" * {w}") - print() - print("Review .claude/CODING_NOTES.md before proceeding. Commit is NOT blocked — " - "fix on next commit if intentional.") - print() + message = ( + f"Coding Notes Pre-commit Check — {len(warnings)} issue(s) found:\n" + + "\n".join(f" * {w}" for w in warnings) + + "\n\nReview .claude/CODING_NOTES.md before proceeding. Commit is NOT blocked — " + "fix on next commit if intentional." + ) + # Plain stdout from a PreToolUse hook is only logged. systemMessage shows the warning to the user; + # additionalContext puts it in front of Claude. Neither blocks the tool call (no permissionDecision). + print(json.dumps({ + "systemMessage": message, + "hookSpecificOutput": {"hookEventName": "PreToolUse", "additionalContext": message}, + })) sys.exit(0) diff --git a/.claude/hooks/test_pre_commit_sp_check.py b/.claude/hooks/test_pre_commit_sp_check.py new file mode 100644 index 0000000..f7457ee --- /dev/null +++ b/.claude/hooks/test_pre_commit_sp_check.py @@ -0,0 +1,83 @@ +"""Tests for the pre-commit hook helpers. Run from the repo root: python -m unittest discover -s .claude/hooks""" + +import json +import os +import subprocess +import sys +import unittest + +sys.path.insert(0, os.path.dirname(__file__)) +import pre_commit_sp_check as hook # noqa: E402 + + +class IsGitCommit(unittest.TestCase): + def test_plain_commit(self): + self.assertTrue(hook._is_git_commit_command('git commit -m "x"')) + + def test_commit_after_add(self): + self.assertTrue(hook._is_git_commit_command('git add a.py && git commit -m "x"')) + self.assertTrue(hook._is_git_commit_command("git add -A; git commit -m x")) + self.assertTrue(hook._is_git_commit_command("git add a || git commit -m x")) + self.assertTrue(hook._is_git_commit_command("git add a\ngit commit -m x")) + + def test_global_options_before_subcommand(self): + self.assertTrue(hook._is_git_commit_command("git -C /tmp/repo -c user.name=x commit -m x")) + + def test_non_commit(self): + self.assertFalse(hook._is_git_commit_command("git status")) + self.assertFalse(hook._is_git_commit_command("git add a.py && git push")) + self.assertFalse(hook._is_git_commit_command("ls -la")) + + def test_commit_word_inside_message_is_not_a_commit(self): + self.assertFalse(hook._is_git_commit_command('git log --grep "git commit"')) + self.assertFalse(hook._is_git_commit_command('echo "run git commit later"')) + + def test_multiline_message_stays_one_command(self): + self.assertTrue(hook._is_git_commit_command('git commit -m "line one\nline two"')) + self.assertFalse(hook._is_git_commit_command('git log --grep "a\ngit commit b"')) + + +class AddedLines(unittest.TestCase): + DIFF = ( + "--- a/x.py\n+++ b/x.py\n@@\n+try:\n+ pass\n+except Exception as e:\n" + "--- a/notes.md\n+++ b/notes.md\n@@\n+ except Exception as e:\n" + ) + + def test_pairs_lines_with_files(self): + pairs = hook._added_lines_by_file(self.DIFF) + self.assertIn(("x.py", "+except Exception as e:"), pairs) + self.assertIn(("notes.md", "+ except Exception as e:"), pairs) + self.assertFalse(any(line.startswith("+++") for _, line in pairs)) + + +class EndToEnd(unittest.TestCase): + def run_hook(self, command, cwd): + payload = json.dumps({"tool_input": {"command": command}}) + return subprocess.run([sys.executable, hook.__file__], input=payload, capture_output=True, + text=True, cwd=cwd) + + def test_warning_is_json_system_message_and_python_only(self): + import tempfile + with tempfile.TemporaryDirectory() as tmp: + def git(*args): + subprocess.run(["git", *args], cwd=tmp, check=True, capture_output=True) + git("init", "-q") + with open(os.path.join(tmp, "notes.md"), "w") as f: + f.write(" except Exception as e:\n") + git("add", "notes.md") + out = self.run_hook("git commit -m x", tmp) + self.assertEqual(out.returncode, 0) + self.assertEqual(out.stdout.strip(), "") # markdown example must not warn + + with open(os.path.join(tmp, "a.py"), "w") as f: + f.write("try:\n pass\nexcept Exception:\n pass\n") + git("add", "a.py") + out = self.run_hook("git add a.py && git commit -m x", tmp) + self.assertEqual(out.returncode, 0) + data = json.loads(out.stdout) + self.assertIn("except Exception", data["systemMessage"]) + self.assertEqual(data["hookSpecificOutput"]["hookEventName"], "PreToolUse") + + +if __name__ == "__main__": + unittest.main()