From ae6b03ffb50365d6029f07ba07cf83ac5dd0e034 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 6 Aug 2026 10:48:06 +0200 Subject: [PATCH 1/5] fix(hooks): read the PreToolUse payload, add three forge gates validate_git_command.py never fired. It read a top-level "command" key, but Claude Code sends {"tool_name": ..., "tool_input": {"command": ...}}. That JSON parses fine, so the JSONDecodeError fallback did not trigger either -- the function got an empty string and returned silently on every invocation. Everyone who installs this skill has been running a hook that does nothing. With the payload actually read, three gates move in from a machine-local hook where they did not belong. All three are git-workflow concerns and one of them recommends this skill's own pr-status.sh, which the skill was not shipping the gate for: - A forge body carrying 3+ hard-wrapped prose lines. The breaks read ragged in the web UI and survive verbatim in release notes. - A reply to repos/O/R/pulls/comments/{id}/replies without the PR number. GitHub answers 404 and posts nothing, silently. - A sleep-loop over gh pr view/checks/status. It waits for the one outcome it was told about and sleeps through every other actionable event. These deny rather than warn, because each describes an action that silently does the wrong thing rather than one that merely reads badly. They run before the advisory checks -- a denied command never executes, so warning about its commit-message style would be noise. A fourth gate stays machine-local: it resolves commit SHAs against the forge to catch hashes written from memory, which needs a network call and would not fit the 2s hook timeout. Signed-off-by: Sebastian Mendel --- scripts/validate_git_command.py | 164 +++++++++++++++++++++++++++++++- 1 file changed, 162 insertions(+), 2 deletions(-) diff --git a/scripts/validate_git_command.py b/scripts/validate_git_command.py index 0e8dde9..617cc89 100755 --- a/scripts/validate_git_command.py +++ b/scripts/validate_git_command.py @@ -44,6 +44,154 @@ ] +# --------------------------------------------------------------------------- +# Gates. Unlike the advisory checks above these refuse the call, because each +# one describes an action that silently does the wrong thing rather than one +# that merely reads badly. +# --------------------------------------------------------------------------- + +# Bodies posted to a forge: gh pr/issue/release create|edit|comment. +FORGE_BODY = re.compile(r"\bgh\s+(pr|issue|release)\s+(create|edit|comment)\b", re.I) +BODY_FILE = re.compile(r"--(?:body|notes)-file[= ]+(\S+)") +BODY_INLINE = re.compile(r"--(?:body|notes)[= ]+(['\"])(.*?)\1", re.S) + +# Replying to a review comment needs the PR number in the path: +# repos/O/R/pulls/{pr}/comments/{id}/replies. Without it GitHub answers 404 and +# the reply is silently not posted. Two deliberate limits: only the /replies +# subresource is checked (`pulls/comments/{id}` is a legitimate read endpoint), +# and only a segment that actually invokes gh/curl counts — matching the path +# anywhere would block writing about it in an echo or a commit message. +REPLY_WITHOUT_PR = re.compile(r"/pulls/comments/[^/\s'\"]+/replies\b") +INVOKES_FORGE_API = re.compile(r"^\s*(?:gh\s+api|curl)\b") + +POLL_LOOP = re.compile(r"\b(?:until|while)\b.*?\bsleep\b", re.DOTALL) +FOR_LOOP_POLL = re.compile(r"\bfor\b[^\n]*\bin\b[^\n]*\bseq\b.*?\bsleep\b", re.DOTALL) +POLLS_PR = re.compile( + r"\bgh\s+pr\s+(?:view|checks|status)\b" + r"|\bgh\s+api\b[^\n]*?/pulls/" + r"|\bpr-status\.sh\b" +) + + +def read_command(data) -> str: + """Pull the command out of a PreToolUse payload. + + Claude Code sends {"tool_name": ..., "tool_input": {"command": ...}}. An + earlier version read a top-level "command" key, which that payload does not + have, so the hook returned silently on every invocation and none of the + checks below ever ran. + """ + if not isinstance(data, dict): + return "" + tool_input = data.get("tool_input") + if isinstance(tool_input, dict) and tool_input.get("command"): + return tool_input["command"] + return data.get("command", "") or "" + + +def deny(reason: str) -> None: + print( + json.dumps( + { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason, + } + } + ) + ) + + +def hard_wrapped(text: str) -> int: + """Count prose lines that look hard-wrapped at a fixed column. + + Only consecutive prose counts: a short line followed by more prose is the + signature of a fixed-width wrap. Tables, lists, quotes, headings, link + references and fenced code keep their own line structure and are skipped, + as is a lone short line (a real one-line paragraph). + """ + lines = text.split("\n") + fenced = False + hits = 0 + for i, ln in enumerate(lines): + s = ln.strip() + if s.startswith("```") or s.startswith("~~~"): + fenced = not fenced + continue + if fenced or not s: + continue + if re.match(r"^([-*+>#|]|\d+[.)]|\[)", s) or "|" in s: + continue + nxt = lines[i + 1].strip() if i + 1 < len(lines) else "" + if not nxt or re.match(r"^([-*+>#|`]|\d+[.)]|\[)", nxt): + continue + # A prose line that stops in the 55-85 column band while the paragraph + # continues on the next line was wrapped by hand, not by the renderer. + if 55 <= len(ln.rstrip()) <= 85: + hits += 1 + return hits + + +def forge_body_hard_wrapped(cmd: str) -> str | None: + if not FORGE_BODY.search(cmd): + return None + bodies = [] + for m in BODY_FILE.finditer(cmd): + p = m.group(1).strip("'\"") + try: + with open(p, encoding="utf-8") as fh: + bodies.append((p, fh.read())) + except OSError: + pass + for m in BODY_INLINE.finditer(cmd): + bodies.append(("--body", m.group(2))) + for name, text in bodies: + n = hard_wrapped(text) + if n >= 3: + return ( + f"{name} carries {n} hard-wrapped prose lines. Bodies posted to " + "GitHub/GitLab/Jira must NOT be wrapped at a fixed column: write " + "each paragraph as ONE long line and let the renderer reflow it. " + "Hard breaks read ragged in the web UI, break on mobile, and " + "corrupt every later quote or diff — and in release notes they " + "survive verbatim, unlike a CHANGELOG where markdown reflows. " + "Tables, lists and fenced code keep their own line structure. " + "(Commit messages are the exception and stay wrapped at ~72.)" + ) + return None + + +def reply_path_without_pr(cmd: str) -> str | None: + for segment in re.split(r"(?:\|\||&&|[;|&\n])", cmd): + if INVOKES_FORGE_API.match(segment) and REPLY_WITHOUT_PR.search(segment): + return ( + "A review-comment reply needs the PR number in the path — this " + "one would 404 and post nothing:\n\n" + " repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies\n\n" + "`repos/{owner}/{repo}/pulls/comments/{comment_id}` (without " + "/replies) is the valid form for READING one comment, which is " + "where the shorter path comes from." + ) + return None + + +def handrolled_pr_poll(cmd: str) -> str | None: + if "--watch" in cmd or not POLLS_PR.search(cmd): + return None + if not (POLL_LOOP.search(cmd) or FOR_LOOP_POLL.search(cmd)): + return None + return ( + "Hand-rolled poll over pull-request state. Use " + "`pr-status.sh -R --watch` instead: it returns at the " + "FIRST actionable event — a check that failed, a review that arrived, a " + "thread that needs an answer — where a loop written here waits for the " + "one outcome it was told about and sleeps through the rest. A loop that " + "exited only on `merge` slept through the review it was waiting for, and " + "the operator had to ask what was happening." + ) + + def check_conventional_commit(message: str) -> str | None: """Validate commit message follows conventional commits.""" if not re.match(CONVENTIONAL_COMMIT_PATTERN, message): @@ -147,11 +295,23 @@ def main(): try: data = json.loads(input_data) - command = data.get("command", "") + command = read_command(data) except (json.JSONDecodeError, TypeError): command = input_data - if not command or "git" not in command.lower(): + if not command: + return + + # Gates that refuse the call outright. Checked before the advisory + # warnings because a denied command never runs, so warning about its + # style would be noise. + for gate in (forge_body_hard_wrapped, reply_path_without_pr, handrolled_pr_poll): + reason = gate(command) + if reason: + deny(reason) + return + + if "git" not in command.lower(): return warnings = check_command(command) From 3ea4a700373e60ecb2821f8f6f84b048f95648b0 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 6 Aug 2026 11:39:27 +0200 Subject: [PATCH 2/5] style: use the spelled-out re flags and a startswith tuple Three ruff findings, all carried over verbatim from the machine-local hook the gates came from: re.I and re.S (FURB167) and a doubled startswith call (PIE810). Local pre-commit passed because CI pins ruff 0.16.0 while the hook run resolves its own. Verified here against the pinned version -- ruff check and ruff format --check both clean -- and the six gate probes still behave: reply-path deny, PR-poll deny, --watch through, hard-wrapped body deny, commit-message reminder, plain command through. Signed-off-by: Sebastian Mendel --- scripts/validate_git_command.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/validate_git_command.py b/scripts/validate_git_command.py index 617cc89..98bace0 100755 --- a/scripts/validate_git_command.py +++ b/scripts/validate_git_command.py @@ -51,9 +51,11 @@ # --------------------------------------------------------------------------- # Bodies posted to a forge: gh pr/issue/release create|edit|comment. -FORGE_BODY = re.compile(r"\bgh\s+(pr|issue|release)\s+(create|edit|comment)\b", re.I) +FORGE_BODY = re.compile( + r"\bgh\s+(pr|issue|release)\s+(create|edit|comment)\b", re.IGNORECASE +) BODY_FILE = re.compile(r"--(?:body|notes)-file[= ]+(\S+)") -BODY_INLINE = re.compile(r"--(?:body|notes)[= ]+(['\"])(.*?)\1", re.S) +BODY_INLINE = re.compile(r"--(?:body|notes)[= ]+(['\"])(.*?)\1", re.DOTALL) # Replying to a review comment needs the PR number in the path: # repos/O/R/pulls/{pr}/comments/{id}/replies. Without it GitHub answers 404 and @@ -116,7 +118,7 @@ def hard_wrapped(text: str) -> int: hits = 0 for i, ln in enumerate(lines): s = ln.strip() - if s.startswith("```") or s.startswith("~~~"): + if s.startswith(("```", "~~~")): fenced = not fenced continue if fenced or not s: From a0bff80282b2285fccd5a94582321d7d4ab0dd4d Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 6 Aug 2026 11:41:26 +0200 Subject: [PATCH 3/5] fix(hooks): never read a non-regular --body-file; add tests Found while reviewing this PR by hand (Copilot could not review it -- the requesting account is over its quota). The hard-wrap gate opened whatever path `--body-file` named. That path is routinely a pipe: `gh pr create --body-file <(generate-body)` hands over /dev/fd/N, and opening it here waits for a writer this process cannot see. Measured before the fix: the hook did not return within 4 seconds against a fifo. A hook that hangs is worse than one that misses a finding, so a non-regular path is now skipped and a regular one is read up to 256 KiB. The script also had no tests at all, which is a poor pairing with gates that deny. tests/test_validate_git_command.py covers twelve cases: the nested-payload bug that made every check unreachable, each gate firing, and -- more useful -- each near-miss that must NOT fire: reading a single comment (same path prefix, no /replies), the reply path quoted inside an echo rather than invoked, a lone `gh pr view` that is not a poll, pr-status.sh --watch, and a single-line body. The fifo case asserts the hook returns at all. Signed-off-by: Sebastian Mendel --- scripts/validate_git_command.py | 14 ++- tests/test_validate_git_command.py | 137 +++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 tests/test_validate_git_command.py diff --git a/scripts/validate_git_command.py b/scripts/validate_git_command.py index 98bace0..4f2e70f 100755 --- a/scripts/validate_git_command.py +++ b/scripts/validate_git_command.py @@ -5,9 +5,14 @@ """ import json +import os import re import sys +# Enough of a body to count wrapped lines in; a cap so an accidentally huge +# file cannot stall the hook. +BODY_READ_LIMIT = 256 * 1024 + # Conventional commit pattern CONVENTIONAL_COMMIT_PATTERN = ( r"^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?!?:\s.+" @@ -142,8 +147,15 @@ def forge_body_hard_wrapped(cmd: str) -> str | None: for m in BODY_FILE.finditer(cmd): p = m.group(1).strip("'\"") try: + # Regular files only, and only the first chunk. `--body-file` can + # name a pipe -- process substitution (`--body-file <(...)`) hands + # over /dev/fd/N -- and reading one here blocks until a writer this + # process cannot see appears. A hook that hangs is worse than one + # that misses a finding, so a non-regular path is skipped. + if not os.path.isfile(p): + continue with open(p, encoding="utf-8") as fh: - bodies.append((p, fh.read())) + bodies.append((p, fh.read(BODY_READ_LIMIT))) except OSError: pass for m in BODY_INLINE.finditer(cmd): diff --git a/tests/test_validate_git_command.py b/tests/test_validate_git_command.py new file mode 100644 index 0000000..222a2a2 --- /dev/null +++ b/tests/test_validate_git_command.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +"""Cases for scripts/validate_git_command.py. + +Run: python3 tests/test_validate_git_command.py + +The gates deny commands, so a regression here silently either blocks +legitimate work or stops catching the thing it was written for. Each case +names the failure it stands for. +""" + +import json +import os +import subprocess +import sys +import tempfile + +HOOK = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), + "scripts", + "validate_git_command.py", +) + +WRAPPED = ( + "A paragraph wrapped by hand at roughly the seventy-two column mark,\n" + "which is the shape this gate exists to catch before it is posted and\n" + "the breaks survive verbatim in the rendered release notes forever.\n" + "A fourth line so the run is unambiguously a wrapped paragraph." +) + +CASES = [ + # (name, expected, command) + # The bug that made every check below unreachable: the payload is nested. + ("nested payload reaches the checks", "REMINDER", 'git commit -m "stuff"'), + ("conventional message stays quiet", "PASS", 'git commit -m "fix: handle null"'), + ( + "reply path without the PR number", + "DENY", + "gh api repos/o/r/pulls/comments/123/replies -f body=x", + ), + # Legitimate read endpoint - same prefix, no /replies. + ( + "reading one comment is allowed", + "PASS", + "gh api repos/o/r/pulls/comments/123", + ), + # Writing ABOUT the path must not be blocked, only invoking it. + ( + "the path inside an echo is not a call", + "PASS", + "echo 'use repos/o/r/pulls/comments/1/replies'", + ), + ( + "sleep-loop over PR state", + "DENY", + "until [ x = y ]; do gh pr view 1 --json state; sleep 30; done", + ), + ( + "pr-status.sh --watch is the fix, not the fault", + "PASS", + "pr-status.sh -R o/r 1 --watch", + ), + ( + "a single gh pr view is not a poll", + "PASS", + "gh pr view 1 --repo o/r --json state", + ), + ("hard-wrapped inline body", "DENY", f'gh pr create --title x --body "{WRAPPED}"'), + ( + "one long line is what we want", + "PASS", + 'gh pr create --title x --body "One long line that the renderer reflows by itself."', + ), + ("unrelated command", "PASS", "ls -la"), +] + + +def run(command: str) -> str: + payload = json.dumps({"tool_name": "Bash", "tool_input": {"command": command}}) + # check=False: a non-zero exit is itself something the cases assert on, + # not a reason to abort the run. + proc = subprocess.run( + [sys.executable, HOOK], + input=payload, + capture_output=True, + text=True, + timeout=10, + check=False, + ) + out = proc.stdout.strip() + if not out: + return "PASS" + if out.startswith("{"): + decision = json.loads(out)["hookSpecificOutput"]["permissionDecision"] + return "DENY" if decision == "deny" else "PASS" + return "REMINDER" + + +def fifo_case() -> bool: + """A --body-file naming a pipe must not block the hook. + + `gh pr create --body-file <(generate)` hands over /dev/fd/N. Opening it + here waits for a writer this process cannot see, and the hook hangs. + """ + tmp = tempfile.mkdtemp() + path = os.path.join(tmp, "fifo") + os.mkfifo(path) + try: + run(f"gh pr create --title x --body-file {path}") + return True + except subprocess.TimeoutExpired: + return False + finally: + os.unlink(path) + os.rmdir(tmp) + + +def main() -> int: + fails = 0 + for name, expected, command in CASES: + got = run(command) + ok = got == expected + fails += 0 if ok else 1 + print(f" {'OK ' if ok else 'FAIL'} {name:<44} want={expected:<9} got={got}") + + ok = fifo_case() + fails += 0 if ok else 1 + print( + f" {'OK ' if ok else 'FAIL'} {'--body-file on a pipe returns':<44} want=no-hang " + f"got={'no-hang' if ok else 'HUNG'}" + ) + + print(f" ---- failures: {fails}") + return 1 if fails else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From f02808c7b9e1b3053b3d9353693f81e47fffe9b7 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 6 Aug 2026 11:42:25 +0200 Subject: [PATCH 4/5] fix(hooks): a command prefix must not bypass the reply gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `env FOO=1 gh api …/replies` and `sudo gh api …/replies` both passed the gate. INVOKES_FORGE_API anchored on gh/curl at the start of the segment, so anything in front of them broke the match. The anchor itself has to stay -- matching the reply path anywhere would block writing ABOUT it in an echo or a commit message, which is how the pattern ended up anchored in the first place. So the anchor now skips over leading VAR=value assignments and the usual wrapper words (sudo, env, time, command, nohup, xargs) before requiring gh api / curl. Quoted prose is unaffected. Four cases added, three of them prefixes that were measured slipping through, plus one for a call in the second segment of a && chain. Signed-off-by: Sebastian Mendel --- scripts/validate_git_command.py | 10 +++++++++- tests/test_validate_git_command.py | 22 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/scripts/validate_git_command.py b/scripts/validate_git_command.py index 4f2e70f..b68bdf0 100755 --- a/scripts/validate_git_command.py +++ b/scripts/validate_git_command.py @@ -69,7 +69,15 @@ # and only a segment that actually invokes gh/curl counts — matching the path # anywhere would block writing about it in an echo or a commit message. REPLY_WITHOUT_PR = re.compile(r"/pulls/comments/[^/\s'\"]+/replies\b") -INVOKES_FORGE_API = re.compile(r"^\s*(?:gh\s+api|curl)\b") +# The anchor matters: matching the path anywhere would block writing ABOUT it +# in an echo or a commit message. But anchoring on gh/curl alone let any +# prefix through -- `env FOO=1 gh api …` and `sudo gh api …` both slipped the +# gate -- so leading assignments and the usual wrapper words are skipped over +# first. Still anchored, so quoted prose stays unaffected. +INVOKES_FORGE_API = re.compile( + r"^\s*(?:(?:[A-Za-z_][A-Za-z0-9_]*=\S*|sudo|env|time|command|nohup|xargs)\s+)*" + r"(?:gh\s+api|curl)\b" +) POLL_LOOP = re.compile(r"\b(?:until|while)\b.*?\bsleep\b", re.DOTALL) FOR_LOOP_POLL = re.compile(r"\bfor\b[^\n]*\bin\b[^\n]*\bseq\b.*?\bsleep\b", re.DOTALL) diff --git a/tests/test_validate_git_command.py b/tests/test_validate_git_command.py index 222a2a2..7ce1878 100644 --- a/tests/test_validate_git_command.py +++ b/tests/test_validate_git_command.py @@ -49,6 +49,28 @@ "PASS", "echo 'use repos/o/r/pulls/comments/1/replies'", ), + # A prefix must not become a bypass: both of these slipped the anchor. + ( + "env assignment before the call", + "DENY", + "env FOO=1 gh api repos/o/r/pulls/comments/1/replies -f body=x", + ), + ( + "sudo before the call", + "DENY", + "sudo gh api repos/o/r/pulls/comments/1/replies -f body=x", + ), + ( + "bare assignment before the call", + "DENY", + "GH_TOKEN=x gh api repos/o/r/pulls/comments/1/replies -f body=x", + ), + # Second segment of a chain still counts. + ( + "after && still counts", + "DENY", + "git status && gh api repos/o/r/pulls/comments/1/replies -f body=x", + ), ( "sleep-loop over PR state", "DENY", From 009866ca8ab0eeabfd8fd9397fdebe16f8e4f8cc Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Thu, 6 Aug 2026 11:45:51 +0200 Subject: [PATCH 5/5] fix(tests): mark the test file executable (EXE001) The new test file carries a shebang but went in as 100644, which ruff 0.16.0 flags as EXE001 in its default rule set. It cannot be reproduced on this machine: DrvFs mounts report every file as executable to stat(), so `ruff check` passes locally regardless of the git mode. Only CI on native ext4 is authoritative here, and `chmod +x` plus `git add` does not reliably carry the bit either -- hence `git update-index --cacheinfo 100755`. Verified in the tree rather than the working directory: `git ls-files -s` reports 100755. Signed-off-by: Sebastian Mendel --- tests/test_validate_git_command.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tests/test_validate_git_command.py diff --git a/tests/test_validate_git_command.py b/tests/test_validate_git_command.py old mode 100644 new mode 100755