diff --git a/scripts/hooks/doc-file-warning.js b/scripts/hooks/doc-file-warning.js index a5ba8230..a8e71262 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 (case-sensitive, consistent with ADHOC_FILENAMES) + if (!/\.(md|txt)$/.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, _options = {}) { + 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 a structured path (e.g. docs/, .claude/, skills/, .github/, benchmarks/, templates/)', + }; + } + + 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..9d966634 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,42 @@ 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, partial-match, and non-standard extension case - NOT on denylist + const allowedNonDenylist = [ + 'random-notes.md', + 'notes.txt', + 'scratch.md', + 'ideas.txt', + '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}`, () => { + 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 +145,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 +177,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 +197,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++);