From 27d71c954840482c6c5a55d2150d1c6470bad61a Mon Sep 17 00:00:00 2001 From: Lidang-Jiang Date: Sun, 29 Mar 2026 09:54:23 +0800 Subject: [PATCH 1/3] fix(hooks): port doc-file-warning denylist policy to current hook runtime Replace the broad allowlist approach with a targeted denylist that only warns on known ad-hoc filenames (NOTES, TODO, SCRATCH, TEMP, DRAFT, BRAINSTORM, SPIKE, DEBUG, WIP) outside structured directories. This eliminates false positives for legitimate markdown-heavy workflows while still catching impulse documentation files. Closes #988 Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Lidang-Jiang --- scripts/hooks/doc-file-warning.js | 85 +++++++++++++++++---------- tests/hooks/doc-file-warning.test.js | 87 +++++++++++++++++++++++----- 2 files changed, 130 insertions(+), 42 deletions(-) diff --git a/scripts/hooks/doc-file-warning.js b/scripts/hooks/doc-file-warning.js index a5ba8230..b0f3eea3 100644 --- a/scripts/hooks/doc-file-warning.js +++ b/scripts/hooks/doc-file-warning.js @@ -1,7 +1,13 @@ #!/usr/bin/env node /** * Doc file warning hook (PreToolUse - Write) - * Warns about non-standard documentation files. + * + * Uses a denylist approach: only warn on known ad-hoc documentation + * filenames (NOTES, TODO, SCRATCH, etc.) outside structured directories. + * This avoids false positives for legitimate markdown-heavy workflows + * (specs, ADRs, command definitions, skill files, etc.). + * + * Policy ported from the intent of PR #962 into the current hook architecture. * Exit code 0 always (warns only, never blocks). */ @@ -12,31 +18,59 @@ const path = require('path'); const MAX_STDIN = 1024 * 1024; let data = ''; -function isAllowedDocPath(filePath) { +// Known ad-hoc filenames that indicate impulse/scratch files (case-sensitive, uppercase only) +const ADHOC_FILENAMES = /^(NOTES|TODO|SCRATCH|TEMP|DRAFT|BRAINSTORM|SPIKE|DEBUG|WIP)\.(md|txt)$/; + +// Structured directories where even ad-hoc names are intentional +const STRUCTURED_DIRS = /(^|\/)(docs|\.claude|\.github|commands|skills|benchmarks|templates|\.history|memory)\//; + +function isSuspiciousDocPath(filePath) { const normalized = filePath.replace(/\\/g, '/'); - const basename = path.basename(filePath); + const basename = path.basename(normalized); - if (!/\.(md|txt)$/i.test(filePath)) return true; + // Only inspect .md and .txt files + if (!/\.(md|txt)$/i.test(basename)) return false; - if (/^(README|CLAUDE|AGENTS|CONTRIBUTING|CHANGELOG|LICENSE|SKILL|MEMORY|WORKLOG)\.md$/i.test(basename)) { - return true; - } + // Only flag known ad-hoc filenames + if (!ADHOC_FILENAMES.test(basename)) return false; - if (/\.claude\/(commands|plans|projects)\//.test(normalized)) { - return true; - } + // Allow ad-hoc names inside structured directories (intentional usage) + if (STRUCTURED_DIRS.test(normalized)) return false; - if (/(^|\/)(docs|skills|\.history|memory)\//.test(normalized)) { - return true; - } - - if (/\.plan\.md$/i.test(basename)) { - return true; - } - - return false; + return true; } +/** + * Exportable run() for in-process execution via run-with-flags.js. + * Avoids the ~50-100ms spawnSync overhead when available. + */ +function run(inputOrRaw) { + let input; + try { + input = typeof inputOrRaw === 'string' + ? (inputOrRaw.trim() ? JSON.parse(inputOrRaw) : {}) + : (inputOrRaw || {}); + } catch { + return { exitCode: 0 }; + } + const filePath = String(input?.tool_input?.file_path || ''); + + if (filePath && isSuspiciousDocPath(filePath)) { + return { + exitCode: 0, + stderr: + '[Hook] WARNING: Ad-hoc documentation filename detected\n' + + `[Hook] File: ${filePath}\n` + + '[Hook] Consider using structured paths: docs/, .claude/commands/, skills/', + }; + } + + return { exitCode: 0 }; +} + +module.exports = { run }; + +// Stdin fallback for spawnSync execution process.stdin.setEncoding('utf8'); process.stdin.on('data', c => { if (data.length < MAX_STDIN) { @@ -46,17 +80,10 @@ process.stdin.on('data', c => { }); process.stdin.on('end', () => { - try { - const input = JSON.parse(data); - const filePath = String(input.tool_input?.file_path || ''); + const result = run(data); - if (filePath && !isAllowedDocPath(filePath)) { - console.error('[Hook] WARNING: Non-standard documentation file detected'); - console.error(`[Hook] File: ${filePath}`); - console.error('[Hook] Consider consolidating into README.md or docs/ directory'); - } - } catch { - // ignore parse errors + if (result.stderr) { + process.stderr.write(result.stderr + '\n'); } process.stdout.write(data); diff --git a/tests/hooks/doc-file-warning.test.js b/tests/hooks/doc-file-warning.test.js index 7c393fb8..fc492145 100644 --- a/tests/hooks/doc-file-warning.test.js +++ b/tests/hooks/doc-file-warning.test.js @@ -29,11 +29,11 @@ function runScript(input) { } function runTests() { - console.log('\n=== Testing doc-file-warning.js ===\n'); + console.log('\n=== Testing doc-file-warning.js (denylist policy) ===\n'); let passed = 0; let failed = 0; - // 1. Allowed standard doc files - no warning in stderr + // 1. Standard doc filenames - never on denylist, no warning const standardFiles = [ 'README.md', 'CLAUDE.md', @@ -53,10 +53,12 @@ function runTests() { }) ? passed++ : failed++); } - // 2. Allowed directory paths - no warning - const allowedDirPaths = [ + // 2. Structured directory paths - no warning even for ad-hoc names + const structuredDirPaths = [ 'docs/foo.md', 'docs/guide/setup.md', + 'docs/TODO.md', + 'docs/specs/NOTES.md', 'skills/bar.md', 'skills/testing/tdd.md', '.history/session.md', @@ -64,9 +66,13 @@ function runTests() { '.claude/commands/deploy.md', '.claude/plans/roadmap.md', '.claude/projects/myproject.md', + '.github/ISSUE_TEMPLATE/bug.md', + 'commands/triage.md', + 'benchmarks/test.md', + 'templates/DRAFT.md', ]; - for (const file of allowedDirPaths) { - (test(`allows directory path: ${file}`, () => { + for (const file of structuredDirPaths) { + (test(`allows structured directory path: ${file}`, () => { const { code, stderr } = runScript({ tool_input: { file_path: file } }); assert.strictEqual(code, 0, `expected exit code 0, got ${code}`); assert.strictEqual(stderr, '', `expected no warning for ${file}, got: ${stderr}`); @@ -96,10 +102,40 @@ function runTests() { }) ? passed++ : failed++); } - // 5. Non-standard doc files - warning in stderr - const nonStandardFiles = ['random-notes.md', 'TODO.md', 'notes.txt', 'scratch.md', 'ideas.txt']; - for (const file of nonStandardFiles) { - (test(`warns on non-standard doc file: ${file}`, () => { + // 5. Lowercase and partial-match filenames - NOT on denylist, no warning + const allowedNonDenylist = [ + 'random-notes.md', + 'notes.txt', + 'scratch.md', + 'ideas.txt', + 'todo-list.md', + 'my-draft.md', + 'meeting-notes.txt', + ]; + for (const file of allowedNonDenylist) { + (test(`allows non-denylist doc file: ${file}`, () => { + const { code, stderr } = runScript({ tool_input: { file_path: file } }); + assert.strictEqual(code, 0); + assert.strictEqual(stderr, '', `expected no warning for ${file}, got: ${stderr}`); + }) ? passed++ : failed++); + } + + // 6. Ad-hoc denylist filenames at root/non-structured paths - SHOULD warn + const deniedFiles = [ + 'NOTES.md', + 'TODO.md', + 'SCRATCH.md', + 'TEMP.md', + 'DRAFT.txt', + 'BRAINSTORM.md', + 'SPIKE.md', + 'DEBUG.md', + 'WIP.txt', + 'src/NOTES.md', + 'lib/TODO.txt', + ]; + for (const file of deniedFiles) { + (test(`warns on ad-hoc denylist file: ${file}`, () => { const { code, stderr } = runScript({ tool_input: { file_path: file } }); assert.strictEqual(code, 0, 'should still exit 0 (warn only)'); assert.ok(stderr.includes('WARNING'), `expected warning in stderr for ${file}, got: ${stderr}`); @@ -107,7 +143,20 @@ function runTests() { }) ? passed++ : failed++); } - // 6. Invalid/empty input - passes through without error + // 7. Windows backslash paths - normalized correctly + (test('allows ad-hoc name in structured dir with backslash path', () => { + const { code, stderr } = runScript({ tool_input: { file_path: 'docs\\specs\\NOTES.md' } }); + assert.strictEqual(code, 0); + assert.strictEqual(stderr, '', 'expected no warning for structured dir with backslash'); + }) ? passed++ : failed++); + + (test('warns on ad-hoc name with backslash in non-structured dir', () => { + const { code, stderr } = runScript({ tool_input: { file_path: 'src\\SCRATCH.md' } }); + assert.strictEqual(code, 0, 'should still exit 0'); + assert.ok(stderr.includes('WARNING'), 'expected warning for non-structured backslash path'); + }) ? passed++ : failed++); + + // 8. Invalid/empty input - passes through without error (test('handles empty object input without error', () => { const { code, stderr } = runScript({}); assert.strictEqual(code, 0); @@ -126,7 +175,19 @@ function runTests() { assert.strictEqual(stderr, '', `expected no warning for empty file_path, got: ${stderr}`); }) ? passed++ : failed++); - // 7. Stdout always contains the original input (pass-through) + // 9. Malformed input - passes through without error + (test('handles non-JSON input without error', () => { + const result = spawnSync('node', [script], { + encoding: 'utf8', + input: 'not-json', + timeout: 10000, + }); + assert.strictEqual(result.status || 0, 0); + assert.strictEqual(result.stderr || '', ''); + assert.strictEqual(result.stdout, 'not-json'); + }) ? passed++ : failed++); + + // 10. Stdout always contains the original input (pass-through) (test('passes through input to stdout for allowed file', () => { const input = { tool_input: { file_path: 'README.md' } }; const { stdout } = runScript(input); @@ -134,7 +195,7 @@ function runTests() { }) ? passed++ : failed++); (test('passes through input to stdout for warned file', () => { - const input = { tool_input: { file_path: 'random-notes.md' } }; + const input = { tool_input: { file_path: 'TODO.md' } }; const { stdout } = runScript(input); assert.strictEqual(stdout, JSON.stringify(input)); }) ? passed++ : failed++); From 3c3781ca4385c5e5d2a4b1407f452567f03c228e Mon Sep 17 00:00:00 2001 From: Lidang-Jiang Date: Sun, 29 Mar 2026 10:09:02 +0800 Subject: [PATCH 2/3] refactor: address reviewer feedback - Add options={} parameter to run() to match run-with-flags.js contract - Remove case-insensitive flag from extension pre-filter for consistency with ADHOC_FILENAMES regex (both now case-sensitive) - Expand warning text to list more structured paths - Add test cases for uppercase extensions (TODO.MD, NOTES.TXT) Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Lidang-Jiang --- scripts/hooks/doc-file-warning.js | 8 ++++---- tests/hooks/doc-file-warning.test.js | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/hooks/doc-file-warning.js b/scripts/hooks/doc-file-warning.js index b0f3eea3..ea685e25 100644 --- a/scripts/hooks/doc-file-warning.js +++ b/scripts/hooks/doc-file-warning.js @@ -28,8 +28,8 @@ function isSuspiciousDocPath(filePath) { const normalized = filePath.replace(/\\/g, '/'); const basename = path.basename(normalized); - // Only inspect .md and .txt files - if (!/\.(md|txt)$/i.test(basename)) return false; + // Only inspect .md and .txt files (case-sensitive, consistent with ADHOC_FILENAMES) + if (!/\.(md|txt)$/.test(basename)) return false; // Only flag known ad-hoc filenames if (!ADHOC_FILENAMES.test(basename)) return false; @@ -44,7 +44,7 @@ function isSuspiciousDocPath(filePath) { * Exportable run() for in-process execution via run-with-flags.js. * Avoids the ~50-100ms spawnSync overhead when available. */ -function run(inputOrRaw) { +function run(inputOrRaw, options = {}) { let input; try { input = typeof inputOrRaw === 'string' @@ -61,7 +61,7 @@ function run(inputOrRaw) { stderr: '[Hook] WARNING: Ad-hoc documentation filename detected\n' + `[Hook] File: ${filePath}\n` + - '[Hook] Consider using structured paths: docs/, .claude/commands/, skills/', + '[Hook] Consider using a structured path (e.g. docs/, .claude/, skills/, .github/, benchmarks/, templates/)', }; } diff --git a/tests/hooks/doc-file-warning.test.js b/tests/hooks/doc-file-warning.test.js index fc492145..9d966634 100644 --- a/tests/hooks/doc-file-warning.test.js +++ b/tests/hooks/doc-file-warning.test.js @@ -102,7 +102,7 @@ function runTests() { }) ? passed++ : failed++); } - // 5. Lowercase and partial-match filenames - NOT on denylist, no warning + // 5. Lowercase, partial-match, and non-standard extension case - NOT on denylist const allowedNonDenylist = [ 'random-notes.md', 'notes.txt', @@ -111,6 +111,8 @@ function runTests() { 'todo-list.md', 'my-draft.md', 'meeting-notes.txt', + 'TODO.MD', + 'NOTES.TXT', ]; for (const file of allowedNonDenylist) { (test(`allows non-denylist doc file: ${file}`, () => { From 74621683773d8e10a453b7d25df30b5ff5493cf3 Mon Sep 17 00:00:00 2001 From: Lidang-Jiang Date: Sun, 29 Mar 2026 10:14:53 +0800 Subject: [PATCH 3/3] fix(lint): prefix unused options parameter with underscore Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Lidang-Jiang --- scripts/hooks/doc-file-warning.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/hooks/doc-file-warning.js b/scripts/hooks/doc-file-warning.js index ea685e25..a8e71262 100644 --- a/scripts/hooks/doc-file-warning.js +++ b/scripts/hooks/doc-file-warning.js @@ -44,7 +44,7 @@ function isSuspiciousDocPath(filePath) { * Exportable run() for in-process execution via run-with-flags.js. * Avoids the ~50-100ms spawnSync overhead when available. */ -function run(inputOrRaw, options = {}) { +function run(inputOrRaw, _options = {}) { let input; try { input = typeof inputOrRaw === 'string'