feat: manifest-driven PreToolUse + PostToolUse hooks (v1.5.0)#9
Conversation
Greptile SummaryThis PR introduces a manifest-driven hook system (
Confidence Score: 3/5Safe to merge for the Bash and Edit blocking rules, but the Slack table rule will silently miss violations in any message that also contains a code block, and the documented env-var kill switch does not work. The dispatchers and eval-rule logic are solid: stdin is captured once, the AND-chain evaluates correctly, and blocking rules will reliably stop tool calls. The slack-tables-no-codeblock false-negative means the rule passes silently whenever any code block appears anywhere in the Slack message body, and CLAUDE_DISABLE_PLUGIN_HOOKS=1 is documented as a kill switch but is never read by any of the three dispatcher scripts — a user following the docs to temporarily disable hooks would find they still fire. hooks/rules/rules.yaml (slack-tables-no-codeblock not_pattern), README.md and the three dispatcher scripts (CLAUDE_DISABLE_PLUGIN_HOOKS), and scripts/build-rules.mjs (message and bypass_marker validation).
|
| Filename | Overview |
|---|---|
| hooks/lib/eval-rule.sh | Core rule evaluator: correct AND-chain logic, but BYPASS_MARKER is interpolated into grep ERE without escaping. |
| hooks/rules/rules.yaml | SSoT rules manifest: slack-tables-no-codeblock has a false-negative flaw, and gcloud-missing-project misses commands with global flags before the subcommand. |
| scripts/build-rules.mjs | YAML→JSON compiler with IP-leak guard: validates id, applies_to, match, action, tests — but omits message field validation and bypass_marker format enforcement. |
| README.md | Documents CLAUDE_DISABLE_PLUGIN_HOOKS=1 as a kill switch but none of the hook scripts implement this env-var check. |
| hooks/pre-bash.sh | Clean thin dispatcher — reads stdin once, fans out to eval-rule.sh for each Bash-targeted rule, aggregates exit codes correctly. |
| hooks/post-slack.sh | PostToolUse dispatcher — correctly discards eval-rule exit codes since the Slack message has already been sent; warn-only semantics preserved. |
| hooks/test-hooks.sh | Parametrized test runner reads fixtures from rules.json; avoids inline payloads that would trigger other installed hooks during testing. |
| .github/workflows/test-hooks.yml | CI workflow verifies rules.json sync and runs smoke tests; path filters are correct and cover all relevant files. |
Sequence Diagram
sequenceDiagram
participant CC as Claude Code
participant D as Dispatcher
participant ER as eval-rule.sh
participant PI as parse-input.sh
participant RJ as rules.json
CC->>D: tool_input JSON (stdin)
D->>RJ: jq select rules by applies_to
RJ-->>D: list of rule IDs
loop for each rule ID
D->>ER: printf INPUT_RAW | bash eval-rule.sh rule_id
ER->>PI: source reads stdin
ER->>RJ: jq resolve rule by id
RJ-->>ER: RULE_JSON
alt bypass_marker present
ER-->>D: exit 0
else AND-chain conditions
alt all conditions hold
ER-->>CC: stderr message
alt action=block
ER-->>D: exit 2
else action=warn
ER-->>D: exit 0
end
else any condition fails
ER-->>D: exit 0
end
end
end
alt any exit 2
D-->>CC: exit 2 blocked
else
D-->>CC: exit 0 proceeds
end
Comments Outside Diff (2)
-
README.md, line 146-149 (link)CLAUDE_DISABLE_PLUGIN_HOOKS=1is documented but never checkedThe README tells users they can disable all hooks by setting
CLAUDE_DISABLE_PLUGIN_HOOKS=1in their shell. None of the three dispatcher scripts (pre-bash.sh,pre-edit.sh,post-slack.sh) noreval-rule.shinspect this env var. A user who follows the docs to disable hooks temporarily will find the hooks still fire on every tool call.Prompt To Fix With AI
This is a comment left during a code review. Path: README.md Line: 146-149 Comment: **`CLAUDE_DISABLE_PLUGIN_HOOKS=1` is documented but never checked** The README tells users they can disable all hooks by setting `CLAUDE_DISABLE_PLUGIN_HOOKS=1` in their shell. None of the three dispatcher scripts (`pre-bash.sh`, `pre-edit.sh`, `post-slack.sh`) nor `eval-rule.sh` inspect this env var. A user who follows the docs to disable hooks temporarily will find the hooks still fire on every tool call. How can I resolve this? If you propose a fix, please make it concise.
-
scripts/build-rules.mjs, line 81-106 (link)Schema validation does not check for required
messagefieldbuild-rules.mjsvalidatesid,applies_to,match,action, andtests, but notmessage. A rule missingmessagewill compile silently; at runtimejq -r '.message'returns"null"which is printed to stderr — confusing and unhelpful.bypass_markercould also be validated to match/^[a-z0-9-]+$/to prevent the ERE-injection issue noted separately.Prompt To Fix With AI
This is a comment left during a code review. Path: scripts/build-rules.mjs Line: 81-106 Comment: **Schema validation does not check for required `message` field** `build-rules.mjs` validates `id`, `applies_to`, `match`, `action`, and `tests`, but not `message`. A rule missing `message` will compile silently; at runtime `jq -r '.message'` returns `"null"` which is printed to stderr — confusing and unhelpful. `bypass_marker` could also be validated to match `/^[a-z0-9-]+$/` to prevent the ERE-injection issue noted separately. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
hooks/rules/rules.yaml:246-249
**`slack-tables-no-codeblock` false-negative: presence anywhere suppresses the warning**
The `not_pattern: '```'` is evaluated against the entire `text` field in a single `grep`. If a Slack message contains any code block (even on an unrelated topic) AND also a bare markdown table outside that block, `grep` finds the backtick fence somewhere in the text, the not-pattern matches, and `eval-rule.sh` exits 0 — the table warning is silently suppressed. A message with an unrelated code block followed by a bare pipe-table will never trigger this rule.
### Issue 2 of 5
README.md:146-149
**`CLAUDE_DISABLE_PLUGIN_HOOKS=1` is documented but never checked**
The README tells users they can disable all hooks by setting `CLAUDE_DISABLE_PLUGIN_HOOKS=1` in their shell. None of the three dispatcher scripts (`pre-bash.sh`, `pre-edit.sh`, `post-slack.sh`) nor `eval-rule.sh` inspect this env var. A user who follows the docs to disable hooks temporarily will find the hooks still fire on every tool call.
### Issue 3 of 5
hooks/lib/eval-rule.sh:61-63
**Unescaped `BYPASS_MARKER` interpolated into ERE pattern**
`$BYPASS_MARKER` is injected verbatim into the grep ERE string. The schema describes bypass markers as "kebab-case" but this constraint is never enforced in `build-rules.mjs`. A future rule author who uses a character that is special in ERE (`.`, `+`, `(`, `[`, etc.) will either silently mismatch real bypass comments or trigger a grep error that `set -uo pipefail` escalates to an unexpected block.
### Issue 4 of 5
hooks/rules/rules.yaml:91-96
**`gcloud-missing-project`: positive pattern misses flags before the subcommand**
The pattern `^[[:space:]]*gcloud[[:space:]]+(compute|sql|run|...)` requires the trigger subcommand to be the first token after `gcloud`. A command like `gcloud --verbosity=debug compute instances list` won't match, so no warning is emitted even though `--project=` is absent — a false-negative for any invocation that places global flags before the subcommand.
### Issue 5 of 5
scripts/build-rules.mjs:81-106
**Schema validation does not check for required `message` field**
`build-rules.mjs` validates `id`, `applies_to`, `match`, `action`, and `tests`, but not `message`. A rule missing `message` will compile silently; at runtime `jq -r '.message'` returns `"null"` which is printed to stderr — confusing and unhelpful. `bypass_marker` could also be validated to match `/^[a-z0-9-]+$/` to prevent the ERE-injection issue noted separately.
Reviews (1): Last reviewed commit: "feat: manifest-driven PreToolUse + PostT..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tool_input: | ||
| file_path: 'plugins/foo/amd/build/main.min.js' | ||
| content: | | ||
| define(['jquery'], function($) { |
There was a problem hiding this comment.
slack-tables-no-codeblock false-negative: presence anywhere suppresses the warning
The not_pattern: '```' is evaluated against the entire text field in a single grep. If a Slack message contains any code block (even on an unrelated topic) AND also a bare markdown table outside that block, grep finds the backtick fence somewhere in the text, the not-pattern matches, and eval-rule.sh exits 0 — the table warning is silently suppressed. A message with an unrelated code block followed by a bare pipe-table will never trigger this rule.
Prompt To Fix With AI
This is a comment left during a code review.
Path: hooks/rules/rules.yaml
Line: 246-249
Comment:
**`slack-tables-no-codeblock` false-negative: presence anywhere suppresses the warning**
The `not_pattern: '```'` is evaluated against the entire `text` field in a single `grep`. If a Slack message contains any code block (even on an unrelated topic) AND also a bare markdown table outside that block, `grep` finds the backtick fence somewhere in the text, the not-pattern matches, and `eval-rule.sh` exits 0 — the table warning is silently suppressed. A message with an unrelated code block followed by a bare pipe-table will never trigger this rule.
How can I resolve this? If you propose a fix, please make it concise.| if printf '%s' "$INPUT_RAW" | grep -qE "(//|#)[[:space:]]*hook-bypass:[[:space:]]*${BYPASS_MARKER}\b"; then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
BYPASS_MARKER interpolated into ERE pattern
$BYPASS_MARKER is injected verbatim into the grep ERE string. The schema describes bypass markers as "kebab-case" but this constraint is never enforced in build-rules.mjs. A future rule author who uses a character that is special in ERE (., +, (, [, etc.) will either silently mismatch real bypass comments or trigger a grep error that set -uo pipefail escalates to an unexpected block.
Prompt To Fix With AI
This is a comment left during a code review.
Path: hooks/lib/eval-rule.sh
Line: 61-63
Comment:
**Unescaped `BYPASS_MARKER` interpolated into ERE pattern**
`$BYPASS_MARKER` is injected verbatim into the grep ERE string. The schema describes bypass markers as "kebab-case" but this constraint is never enforced in `build-rules.mjs`. A future rule author who uses a character that is special in ERE (`.`, `+`, `(`, `[`, etc.) will either silently mismatch real bypass comments or trigger a grep error that `set -uo pipefail` escalates to an unexpected block.
How can I resolve this? If you propose a fix, please make it concise.| tests: | ||
| - name: warns-on-compute-without-project | ||
| input: | ||
| tool_input: | ||
| command: 'gcloud compute instances list' | ||
| expected_exit: 0 |
There was a problem hiding this comment.
gcloud-missing-project: positive pattern misses flags before the subcommand
The pattern ^[[:space:]]*gcloud[[:space:]]+(compute|sql|run|...) requires the trigger subcommand to be the first token after gcloud. A command like gcloud --verbosity=debug compute instances list won't match, so no warning is emitted even though --project= is absent — a false-negative for any invocation that places global flags before the subcommand.
Prompt To Fix With AI
This is a comment left during a code review.
Path: hooks/rules/rules.yaml
Line: 91-96
Comment:
**`gcloud-missing-project`: positive pattern misses flags before the subcommand**
The pattern `^[[:space:]]*gcloud[[:space:]]+(compute|sql|run|...)` requires the trigger subcommand to be the first token after `gcloud`. A command like `gcloud --verbosity=debug compute instances list` won't match, so no warning is emitted even though `--project=` is absent — a false-negative for any invocation that places global flags before the subcommand.
How can I resolve this? If you propose a fix, please make it concise.
Not yet wired to Claude Code (no hooks.json, dispatchers pending). Branch held while v1.4.1 cleanup PR ships first.
Tier 1 ships 10 deterministic rules enforcing org-wide dev discipline:
PreToolUse on Bash (block):
- ssh-db-mutation gcloud compute ssh + inline DB/Moodle commands
- prod-ops-no-approval --project=*-prod and ENV=prod operations
- destructive-db-ops supabase db reset/push/repair, psql DROP/TRUNCATE
- manual-edge-fn-deploy supabase functions deploy (CI-only)
- gcloud-missing-project (warn) gcloud subcommands missing --project=
PreToolUse on Edit/Write/MultiEdit (block):
- minified-build-output amd/build/*.min.js or dist/*.min.{js,css}
- secrets-hardcoded hardcoded credential patterns
PostToolUse on Slack messages (warn-only):
- slack-unicode-bullets bullets •◦▪▫
- slack-tables-no-codeblock markdown tables outside code blocks
- slack-spanish-tildes tildeless Spanish words
Architecture:
- hooks/rules/rules.yaml is the SSoT for every rule (one row, atomic).
- scripts/build-rules.mjs compiles rules.yaml -> rules.json (the runtime
artifact). CI verifies rules.json is in sync via git diff --exit-code.
- hooks/lib/{parse-input,eval-rule}.sh are generic helpers reused across
all dispatchers — adding a new rule is editing rules.yaml, never
touching shell code.
- hooks/{pre-bash,pre-edit,post-slack}.sh are thin dispatchers (~30 lines
each) that select rules by applies_to and pipe the same payload.
- hooks/test-hooks.sh is parametrized — reads each rule's tests array
from rules.json and asserts expected exit + stderr.
Bypass markers (// hook-bypass: <id>) provide explicit acknowledgement
when a block is incorrect for a specific case. Documented per-rule and in
the README.
Why YAML not hardcoded: applies the DOJ-3698 medicine ("the canon has to
be code that fails the build, or it doesn't hold") to the hooks ecosystem
itself. 1NF (atomic rows), DRY (one place per rule), SSoT (rules.yaml is
canonical), Conway's Law (structure reflects rule taxonomy, not the
Claude Code matcher API).
IP-leak guard: scripts/build-rules.mjs reads .private/forbidden-names.txt
(gitignored) and fails the build if any rule contains a listed name.
The list lives gitignored so the public BSL-1.1 toolkit never publishes
client/org names; each contributor maintains their own.
Fixes 5 issues raised by Greptile reviewer: 1. slack-tables-no-codeblock false-negative — the not_pattern '```' matched any code block anywhere in the message, silently suppressing the warning when an unrelated code block coexisted with a bare pipe-table. Replaced with a tighter positive pattern that detects the distinctive markdown-table separator row (|---|---|), which is far more specific to actual tables. The not_pattern is removed; warn-only tolerates the residual false-positive on tables wrapped in fenced code blocks. 2. CLAUDE_DISABLE_PLUGIN_HOOKS=1 was documented but never implemented. Added the env-var check to the top of all three dispatchers (pre-bash.sh, pre-edit.sh, post-slack.sh). Verified the kill switch correctly bypasses a normally-blocking input. 3. BYPASS_MARKER ERE injection — the marker was interpolated verbatim into a grep pattern. build-rules.mjs now enforces bypass_marker matches ^[a-z0-9-]+$ at compile time. eval-rule.sh adds defense in depth: re-checks the format before using the marker in grep, and logs a stderr warning + ignores the bypass if the format is invalid. 4. gcloud-missing-project pattern missed commands with global flags before the subcommand (e.g., `gcloud --verbosity=debug compute instances list`). Pattern now allows zero or more --flag(=value)? between gcloud and the subcommand. Same allowance applied to the not_pattern's exemption list (version|help|info|config). Added a regression test. 5. build-rules.mjs schema validation — added required-field check for `message` (non-empty string), and bypass_marker format enforcement (must be null/absent OR kebab-case ^[a-z0-9-]+$) per item 3 above. Test count: 34 → 35 (one new test for global-flag handling). All passing.
Greptile's auto-re-review hook didn't fire after the fix commit 2049fb7. This empty commit forces a fresh webhook so the latest score reflects the addressed feedback.
lapc506
force-pushed
the
feat/preToolUse-hooks
branch
from
c0fc7b5 to
2de0304
Compare
1 participant
Why
After repeated incidents where memory-based guidelines (
feedback_*.md) were not enough to prevent rule violations (e.g., SSH+inline DB ops against Critical Rule #1 of a project's CLAUDE.md), this PR introduces deterministic enforcement at tool-call time.Designed by applying the same medicine as the drift-disease meta-spike: "el canon tiene que ser código que falle el build, o no aguanta" — to the hooks ecosystem itself. 1NF / DRY / SSoT / Conway's Law respected.
What ships (Tier 1, 10 rules)
PreToolUse on
Bash(block):ssh-db-mutation— gcloud SSH with inline DB/Moodle commandsprod-ops-no-approval—--project=*-prod/ENV=prodoperationsdestructive-db-ops— supabase db reset/push/repair, psql DROP/TRUNCATE/DELETEmanual-edge-fn-deploy— supabase functions deploy (CI-only)gcloud-missing-project(warn) — gcloud subcommands without--project=PreToolUse on
Edit | Write | MultiEdit(block):minified-build-output— minified content to amd/build or dist directoriessecrets-hardcoded— hardcoded credential patternsPostToolUse on Slack messages (warn-only):
slack-unicode-bullets— bullet charsslack-tables-no-codeblock— markdown tables outside code-block fencesslack-spanish-tildes— common Spanish words missing tildesEach blocking rule has a
bypass_marker. Add// hook-bypass: <marker>to acknowledge.Architecture
hooks/rules/rules.yaml— SSoT. Humans edit. Each rule is one atomic row.scripts/build-rules.mjs— compiles tohooks/rules/rules.json(runtime artifact). CI verifies sync viagit diff --exit-code.hooks/lib/{parse-input,eval-rule}.sh— generic helpers. Adding a rule = editing YAML, never touching shell.hooks/{pre-bash,pre-edit,post-slack}.sh— thin dispatchers (~30 lines each) that select rules byapplies_toand pipe the payload.hooks/test-hooks.sh— parametrized; reads each rule'stestsarray fromrules.jsonand asserts expected exit + stderr.Tier 2 (NOT in this PR)
The repo's contributor guide (
hooks/rules/README.md) documents 7 techniques (T1-T7) for converting non-deterministic memories into structural rules. Tier 2 will ship those (~24 additional rules) once Tier 1 is field-tested.IP-leak guard
scripts/build-rules.mjsreads.private/forbidden-names.txt(gitignored — each contributor maintains their own list) and fails the build if any rule contains a listed client/org name. Public BSL-1.1 toolkit never publishes the names.Dependency
This PR builds on top of #8 (chore: genericize toolkit examples → v1.4.1). Merge #8 first; this branch will rebase cleanly onto main (only
package.jsonand.claude-plugin/plugin.jsonversions conflict — resolution: keep 1.5.0 from this PR).Test plan
npm run build-rulessucceeds + IP-leak guard passes locallynpm run test-hooks— 34/34 tests pass across all 10 rulestest-hooks.ymlpasses on PRclaude plugin update make-no-mistakes~/.claude/hooks/script and corresponding entry from~/.claude/settings.jsonBumps
.claude-plugin/plugin.json1.4.0 → 1.5.0package.json1.4.0 → 1.5.0package.jsonaddsjs-yamlto devDependencies and 2 new npm scripts (build-rules,test-hooks)package.jsonaddshooks/to thefilesarray (so npm/OpenCode distribution includes them)Generated with Claude Code