Skip to content

Commit 9503e5a

Browse files
amondnetamondnet
authored
feat(hooks): add if field to markitdown and deny-vendor-write hooks (#124)
* docs(track): add hooks-if-field-20260328 * feat(hooks): add if field to markitdown and deny-vendor-write hooks Add permission rule syntax filters to reduce unnecessary hook process spawns. Markitdown hook now only fires on binary doc extensions (pptx/docx/xlsx/xls/ppt/doc). Deny-vendor-write hook now only fires on edits targeting vendor/ or sources/ paths. * chore(track): link PR #124 to hooks-if-field-20260328 * fix(hooks): use single-pattern if fields, descope settings hook Permission rule syntax (gitignore spec) does not support | alternation. Split markitdown hook into 6 separate entries, one per extension. Remove if field from deny-vendor-write hook — would require 8 entries (4 tool types × 2 paths), too verbose for the benefit.
1 parent a2bffb3 commit 9503e5a

5 files changed

Lines changed: 179 additions & 0 deletions

File tree

‎.please/docs/tracks/active/hooks-if-field-20260328/metadata.json‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
{
2+
"track_id": "hooks-if-field-20260328",
3+
"type": "feature",
4+
"status": "in_progress",
5+
"created_at": "2026-03-28T22:47:18+09:00",
6+
"updated_at": "2026-03-28T22:48:00+09:00",
7+
"issue": "",
8+
"pr": "#124",
9+
"project": ""
10+
}

‎.please/docs/tracks/active/hooks-if-field-20260328/plan.md‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
# Plan: Add `if` Field to Hooks
2+
3+
> Track: hooks-if-field-20260328
4+
> Spec: [spec.md](./spec.md)
5+
6+
## Overview
7+
8+
- **Source**: /please:plan
9+
- **Track**: hooks-if-field-20260328
10+
- **Created**: 2026-03-28
11+
- **Approach**: Targeted application — add `if` field to 2 hooks with clear performance benefit; skip 4 hooks where benefit is minimal or inapplicable.
12+
13+
## Purpose
14+
15+
Reduce unnecessary hook process invocations by adding the `if` field (permission rule syntax filter) to hooks where it meaningfully narrows scope. The `if` field prevents the hook process from spawning when the tool call doesn't match the pattern, providing a performance benefit for frequently-fired hooks.
16+
17+
## Context
18+
19+
Claude Code hooks support an `if` field that uses permission rule syntax with alternation (`|`) and glob patterns. It acts as a fine-grained filter within a matched group:
20+
1. `matcher` (coarse) — filters by tool name
21+
2. `if` (fine) — filters by tool name + arguments, avoids process spawn if no match
22+
23+
Currently, **no hooks in this codebase use the `if` field**.
24+
25+
## Architecture Decision
26+
27+
**Apply `if` to 2 hooks with clear benefit:**
28+
- **Markitdown** PreToolUse (matcher: `Read`) — fires on every file read, but only handles 6 binary doc extensions
29+
- **Project settings** deny-vendor-write (matcher: `Write|Edit|MultiEdit|NotebookEdit`) — fires on every edit, but only blocks vendor/ and sources/ paths
30+
31+
**Skip 4 hooks:**
32+
- **Worktree** deny-parent-access — parent path is dynamic/runtime, can't pre-filter with static `if`
33+
- **Gatekeeper** PreToolUse — script has complex DENY/ALLOW rules, `if` would duplicate logic
34+
- **Gatekeeper** PermissionRequest — already rare (only fires on unhandled PermissionRequest events)
35+
- **Fetch** PostToolUseFailure — already rare (only fires on WebFetch failures)
36+
37+
## Tasks
38+
39+
### Phase 1: Apply `if` field to hooks
40+
41+
- T-1: Add `if` field to markitdown PreToolUse hook
42+
- File: `plugins/markitdown/hooks/hooks.json`
43+
- Approach: One hook entry per extension (permission rules don't support `|` alternation)
44+
- Patterns: `Read(*.pptx)`, `Read(*.docx)`, `Read(*.xlsx)`, `Read(*.xls)`, `Read(*.ppt)`, `Read(*.doc)`
45+
- Test: Validate with `claude plugin validate plugins/markitdown`
46+
47+
- T-2: ~~Add `if` field to project settings deny-vendor-write hook~~ **DESCOPED**
48+
- File: `.claude/settings.json`
49+
- Reason: Would require 8 separate hook entries (4 tool types × 2 paths) since `|` is not supported. Too verbose for the benefit; the script already handles filtering efficiently.
50+
51+
- T-3: Document evaluation of skipped hooks in spec as decisions
52+
53+
## Key Files
54+
55+
| File | Role |
56+
|------|------|
57+
| `plugins/markitdown/hooks/hooks.json` | Markitdown hook config (modify) |
58+
| `.claude/settings.json` | Project settings with hooks (modify) |
59+
| `plugins/markitdown/hooks/check-binary-doc.sh` | Script that handles binary docs (read-only reference) |
60+
| `hooks/deny-vendor-write.sh` | Script that blocks vendor writes (read-only reference) |
61+
62+
## Verification
63+
64+
- [ ] Markitdown hook no longer fires on plain text/code file reads
65+
- [ ] deny-vendor-write hook no longer fires on edits outside vendor/sources directories
66+
- [ ] All modified hooks pass plugin validation
67+
- [ ] No regression in hook behavior for target cases
68+
69+
## Progress
70+
71+
| Task | Status |
72+
|------|--------|
73+
| T-1: markitdown `if` field | completed |
74+
| T-2: settings `if` field | descoped — `|` not supported, 8 entries too verbose |
75+
| T-3: document skipped hooks | completed |
76+
77+
## Decision Log
78+
79+
| Decision | Rationale |
80+
|----------|-----------|
81+
| Skip worktree hook | Parent path is dynamic — static `if` pattern can't pre-filter |
82+
| Skip gatekeeper hooks | Script-level pattern matching is more flexible; `if` would duplicate logic |
83+
| Skip fetch hook | PostToolUseFailure on WebFetch is already very rare |
84+
| Descope settings hook | `|` alternation not supported in permission rules; 8 entries (4 tools × 2 paths) too verbose |
85+
| One entry per extension | Permission rules use gitignore spec — no alternation, each `if` takes a single pattern |
86+
87+
## Surprises & Discoveries
88+
89+
- Permission rule syntax follows gitignore spec — `|` alternation is NOT supported
90+
- Each `if` field takes exactly one pattern (e.g., `Read(*.docx)`)
91+
- Multiple hook entries in the same `hooks` array with different `if` patterns achieves OR behavior
92+
- `if` is evaluated before process spawn, providing genuine performance benefit
93+
- For multi-tool matchers (Write|Edit|...) with multiple paths, `if` becomes impractical without alternation

‎.please/docs/tracks/active/hooks-if-field-20260328/spec.md‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# Add `if` Field to Hooks
2+
3+
> Track: hooks-if-field-20260328
4+
5+
## Overview
6+
7+
Add the `if` field (permission rule syntax filter) to hooks that would benefit from more granular tool-call filtering. The `if` field narrows when a hook fires beyond the basic `matcher`, reducing unnecessary hook invocations and improving performance.
8+
9+
## Background
10+
11+
Claude Code hooks support an `if` field that uses permission rule syntax (e.g., `"Bash(git *)"`, `"Edit(*.ts)"`) to filter when a hook runs. This field only applies to tool events: PreToolUse, PostToolUse, PostToolUseFailure, and PermissionRequest. Currently, **no hooks in this codebase use the `if` field**.
12+
13+
## Requirements
14+
15+
### Functional Requirements
16+
17+
- [ ] FR-1: Add `if` field to **markitdown** PreToolUse hook to filter `Read` calls to binary document extensions only (e.g., `*.docx`, `*.pptx`, `*.xlsx`, `*.xls`, `*.doc`, `*.ppt`)
18+
- [ ] FR-2: Add `if` field to **project settings** PreToolUse hook (`deny-vendor-write.sh`) to filter edit operations to `vendor/` and `sources/` paths only
19+
- [ ] FR-3: Add `if` field to **worktree** PreToolUse hook (`deny-parent-access.ts`) to narrow file operation scope where beneficial
20+
- [ ] FR-4: Evaluate gatekeeper and fetch hooks — only add `if` if it meaningfully reduces false invocations without conflicting with existing script logic
21+
22+
### Non-functional Requirements
23+
24+
- [ ] NFR-1: All `if` patterns must use valid permission rule syntax as documented by Claude Code
25+
- [ ] NFR-2: No behavioral change — hooks must fire for the same effective cases as before, just with fewer unnecessary invocations
26+
- [ ] NFR-3: Changes must pass `claude plugin validate` for affected plugins
27+
28+
## Acceptance Criteria
29+
30+
- [ ] AC-1: Markitdown hook no longer fires on plain text/code file reads
31+
- [ ] AC-2: deny-vendor-write hook no longer fires on edits outside vendor/sources directories
32+
- [ ] AC-3: All modified hooks pass plugin validation
33+
- [ ] AC-4: No regression in hook behavior for actual target cases
34+
35+
## Out of Scope
36+
37+
- SessionStart hooks (not tool events, `if` field not applicable)
38+
- Adding new hooks or changing existing hook logic/scripts
39+
- Modifying external-plugins (submodule repos maintained separately)
40+
41+
## Assumptions
42+
43+
- Permission rule syntax supports glob patterns with `*` for file path matching in Read/Edit/Write tools
44+
- The `if` field is evaluated before the hook command/script runs, providing a performance benefit

‎.please/docs/tracks/index.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
| [nuxt-ui-hook-guidance-20260328](active/nuxt-ui-hook-guidance-20260328/plan.md) | Nuxt UI Hook Guidance | feature | TBD | 2026-03-28 | in_progress |
1010
| [nuxt-session-hook-20260328](active/nuxt-session-hook-20260328/) | Plugin Recommender | feature | — | 2026-03-28 | in_progress |
1111
| [add-code-intelligence-marketplace-20260328](active/add-code-intelligence-marketplace-20260328/plan.md) | Add code-intelligence marketplace | feature | - | 2026-03-28 | planned |
12+
| [hooks-if-field-20260328](active/hooks-if-field-20260328/) | Add `if` field to hooks | feature | — | 2026-03-28 | planned |
1213

1314
## Recently Completed
1415

‎plugins/markitdown/hooks/hooks.json‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,37 @@
77
{
88
"type": "command",
99
"command": "${CLAUDE_PLUGIN_ROOT}/hooks/check-binary-doc.sh",
10+
"if": "Read(*.pptx)",
11+
"timeout": 5
12+
},
13+
{
14+
"type": "command",
15+
"command": "${CLAUDE_PLUGIN_ROOT}/hooks/check-binary-doc.sh",
16+
"if": "Read(*.docx)",
17+
"timeout": 5
18+
},
19+
{
20+
"type": "command",
21+
"command": "${CLAUDE_PLUGIN_ROOT}/hooks/check-binary-doc.sh",
22+
"if": "Read(*.xlsx)",
23+
"timeout": 5
24+
},
25+
{
26+
"type": "command",
27+
"command": "${CLAUDE_PLUGIN_ROOT}/hooks/check-binary-doc.sh",
28+
"if": "Read(*.xls)",
29+
"timeout": 5
30+
},
31+
{
32+
"type": "command",
33+
"command": "${CLAUDE_PLUGIN_ROOT}/hooks/check-binary-doc.sh",
34+
"if": "Read(*.ppt)",
35+
"timeout": 5
36+
},
37+
{
38+
"type": "command",
39+
"command": "${CLAUDE_PLUGIN_ROOT}/hooks/check-binary-doc.sh",
40+
"if": "Read(*.doc)",
1041
"timeout": 5
1142
}
1243
]

0 commit comments

Comments
 (0)