fix(hooks): replace Write hook allowlist with targeted denylist#962
fix(hooks): replace Write hook allowlist with targeted denylist#962Zandereins wants to merge 2 commits intoaffaan-m:mainfrom
Conversation
The current PreToolUse Write hook blocks ALL .md/.txt files except a hardcoded allowlist (README, CLAUDE, AGENTS, CONTRIBUTING, .claude/plans/, .planning/). This causes false positives for legitimate workflows: - docs/specs/ and docs/adr/ (spec-driven development) - commands/ and skills/ (.md-based skill/command definitions) - .github/ (issue templates, PR templates) - benchmarks/ (.md test fixtures) - .claude/*/memory/ (auto-memory plugin files) The new approach flips the logic: instead of blocking everything and allowlisting paths, it only blocks known ad-hoc filenames (NOTES, TODO, SCRATCH, TEMP, DRAFT, BRAINSTORM, SPIKE, DEBUG, WIP) and exempts structured directories (docs/, .claude/, .github/, commands/, skills/, benchmarks/, templates/). Benefits: - Future-proof: new structured paths work without hook updates - Shorter regex (142 vs 276 chars for equivalent allowlist) - Targets the actual anti-pattern (impulse docs) not the file extension - 25/25 test cases verified (6 blocked, 19 allowed including edge cases) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated the PreToolUse/Write hook in Changes
Sequence Diagram(s)(omitted — change is a single-file hook logic update and does not introduce multi-component sequential flow) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces the Key observations:
Confidence Score: 5/5Safe to merge — all previously flagged P1 issues are resolved and no new blocking issues were found. Both prior P1 concerns (relative-path allowlist bypass and unused No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([Write tool fires]) --> B{Extension\n.md or .txt?}
B -- No --> Z([ALLOWED])
B -- Yes --> C{Basename matches\ndenylist?\nNOTES/TODO/SCRATCH/\nTEMP/DRAFT/BRAINSTORM/\nSPIKE/DEBUG/WIP}
C -- No --> Z
C -- Yes --> D{Path contains\nstructured dir?\ndocs/ .claude/ .github/\ncommands/ skills/\nbenchmarks/ templates/}
D -- Yes --> Z
D -- No --> E([BLOCKED\nexit 2])
Reviews (2): Last reviewed commit: "fix(hooks): handle relative paths and no..." | Re-trigger Greptile |
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/hooks.json">
<violation number="1" location="hooks/hooks.json:40">
P1: Write hook path checks are not normalized, causing relative-path false blocks and Windows-path denylist bypass.</violation>
<violation number="2" location="hooks/hooks.json:40">
P1: The structured-path exemption regex only matches directories preceded by `/`, so relative paths like `docs/specs/NOTES.md` are incorrectly blocked. Update the pattern to match either start-of-string or `/` before the directory segment.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/hooks.json`:
- Line 40: The hook's inline command uses slash-specific checks
(split('/').pop(),
/\\/(docs|\\.claude|\\.github|commands|skills|benchmarks|templates)\\//i and a
leading `/`) which mis-handle relative and Windows paths; normalize input paths
before testing (e.g., replace backslashes with forward slashes or use a basename
extractor) and update the directory regex to not require a leading slash so
relative paths match (ensure the basename extraction handles both `\` and `/`
and the directory membership check tests the normalized path for any of
docs/.claude/.github/commands/skills/benchmarks/templates anywhere in the path).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Zandereins
left a comment
There was a problem hiding this comment.
Self-Review: Critical regression found
All three review bots (Greptile, Cubic, CodeRabbit) independently identified the same bug — they're right.
Bug: Relative paths bypass the structured-path exemption
The exemption regex \/(docs|\.claude|\.github|commands|skills|benchmarks|templates)\/ requires a leading / before the directory name. Relative paths (the common case in Claude Code) start with docs/, not /docs/, so they never match the exemption and get incorrectly blocked.
Trace for docs/specs/NOTES.md:
\.(md|txt)$→ ✅ matchsplit('/').pop()→NOTES.md→ ✅ denylist match\/(docs|...)\/on"docs/specs/NOTES.md"→ ❌ no leading/→ exemption fails- Result: BLOCKED (should be allowed)
Required fixes before merge
- Regex: Replace
\/with(^|\/)so both absolute and relative paths match - Cleanup: Remove unused
require('fs') - Nice-to-have: Normalize Windows backslashes before regex checks
Will push a fix commit.
…hook
- Fix structured-path exemption regex to match relative paths (docs/specs/NOTES.md)
by using (^|\/) instead of \/ which required a leading slash
- Normalize Windows backslashes before regex checks for cross-platform safety
- Keep denylist case-sensitive (only block UPPERCASE ad-hoc names like NOTES.md,
not intentional lowercase notes.md)
- Remove unused require('fs')
Addresses review feedback from Greptile, Cubic, and CodeRabbit on PR affaan-m#962.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Closing as stale. The policy idea is still valid, but this PR edits obsolete hook wiring and is not mergeable against the current hook runtime. Tracked cleanly in follow-up issue #988, which ports the denylist idea to scripts/hooks/doc-file-warning.js and current tests. |
Description
The current
PreToolUseWrite hook blocks all.md/.txtfiles unless they match a hardcoded allowlist (README.md,CLAUDE.md,AGENTS.md,CONTRIBUTING.md,.claude/plans/,.planning/). This causes false positives for users with legitimate markdown-heavy workflows:docs/specs/*.mddocs/adr/*.mdcommands/*.mdskills/*.md.github/*.mdbenchmarks/*.md.claude/*/memory/*.mdThe fix
Instead of allowlisting paths (breaks on every new directory), this PR denylists filenames — blocking only known ad-hoc anti-patterns:
NOTES.md,TODO.md,SCRATCH.md,TEMP.md,DRAFT.md,BRAINSTORM.md,SPIKE.md,DEBUG.md,WIP.mdThese are still allowed in structured directories (
docs/,.claude/,.github/,commands/,skills/,benchmarks/,templates/) since context matters —docs/specs/NOTES.mdis intentional,~/project/NOTES.mdis impulse.Why denylist > allowlist
Type of Change
Testing
25/25 test cases verified:
Correctly blocked (6):
~/project/NOTES.md,TODO.txt,SCRATCH.md,DRAFT.md,WIP.txt,BRAINSTORM.mdCorrectly allowed (19):
docs/specs/v7.1-plan.md,docs/adr/001.md,README.md,CLAUDE.md,CHANGELOG.md,SECURITY.md,commands/triage.md,skills/SKILL.md,.claude/memory/feedback.md,.github/ISSUE_TEMPLATE/bug.md,benchmarks/test.md,LICENSE.txtdocs/specs/NOTES.md✅ allowed,notes.md(lowercase) ✅ allowed,todo-list.md✅ allowedChecklist
🤖 Generated with Claude Code
Summary by cubic
Switches the Write hook from a broad .md/.txt allowlist to a targeted denylist of ad‑hoc filenames. Adds relative path support and Windows path normalization to reduce false positives while still blocking scratch files.
NOTES|TODO|SCRATCH|TEMP|DRAFT|BRAINSTORM|SPIKE|DEBUG|WIP(UPPERCASE only) outside structured paths.docs/,.claude/,.github/,commands/,skills/,benchmarks/,templates/; matcher now handles relative paths and normalizes backslashes on Windows.Written for commit 39fa933. Summary will update on new commits.
Summary by CodeRabbit