fix(hooks): read the PreToolUse payload, add three forge gates - #145
Conversation
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 <github@sebastianmendel.de>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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 <github@sebastianmendel.de>
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 <github@sebastianmendel.de>
`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 <github@sebastianmendel.de>
Review by hand — Copilot could not run
Fixed while reviewing
A command prefix bypassed the reply gate. No tests existed for this script at all, which pairs badly with gates that deny. Sixteen cases now, and the useful half is the near-misses that must not fire: reading a single comment (same path prefix, no Limitations I am not fixing here
The 55–85 column band is a heuristic. A paragraph whose lines genuinely fall in that range, three times consecutively, is denied. The
These three gates now exist in two places. They still run in the machine-local hook they came from. Until that copy is stripped, a matching command gets two denies; the first wins, so the effect is cosmetic. I am deliberately not removing them before this merges — a window with no gate is the worse failure, and this repo has just demonstrated how long a silently-inactive hook can go unnoticed. What I checked and found sound
Evidence statusEvery deny and pass above is a test case in |
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 <github@sebastianmendel.de>
|
scripts/validate_git_command.pyhas never fired for anyone who installs this skill. It reads a top-level"command"key, but the PreToolUse payload is{"tool_name": ..., "tool_input": {"command": ...}}. That JSON parses cleanly, so theJSONDecodeErrorfallback does not rescue it either —commandcomes out empty and the function returns before any check runs.With
read_command()accepting both shapes the existing advisory checks work as intended. That fix is the reason for the rest of the PR: three gates that belong to this skill had been living in a machine-local hook, invisible to anyone installing git-workflow. One of them recommends this repo's ownscripts/pr-status.sh, so the skill was shipping the tool without the gate that points at it.What the three gates catch
repos/O/R/pulls/comments/{id}/replieswithout the PR numbergh pr view/checks/statusThey deny rather than warn: each describes an action that silently does the wrong thing, as opposed to one that merely reads badly. They run before the advisory checks, since a denied command never executes and warning about its commit-message style would be noise.
Deliberate limits. The reply gate only matches the
/repliessubresource and only in a segment that actually invokesgh/curl—pulls/comments/{id}is a legitimate read endpoint, and matching the path anywhere would block writing about it in anecho. The poll gate exempts--watch. The hard-wrap counter skips tables, lists, quotes, headings and fenced code, and ignores a lone short line, so a real one-line paragraph does not trip it.A fourth gate stays machine-local on purpose: it resolves commit SHAs against the forge to catch hashes written from memory, which needs a network call and would not fit the 2 s hook timeout in
hooks/hooks.json.Verification
Ruff format and ruff check pass via pre-commit.