Merge pull request #992 from Lidang-Jiang/fix/doc-file-warning-denylist

fix(hooks): port doc-file-warning denylist policy to current hook runtime

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;
+
+  // Only flag known ad-hoc filenames
+  if (!ADHOC_FILENAMES.test(basename)) return false;
+
+  // Allow ad-hoc names inside structured directories (intentional usage)
+  if (STRUCTURED_DIRS.test(normalized)) return false;
 
-  if (/^(README|CLAUDE|AGENTS|CONTRIBUTING|CHANGELOG|LICENSE|SKILL|MEMORY|WORKLOG)\.md$/i.test(basename)) {
   return true;
 }
 
-  if (/\.claude\/(commands|plans|projects)\//.test(normalized)) {
-    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/)',
+    };
   }
 
-  if (/(^|\/)(docs|skills|\.history|memory)\//.test(normalized)) {
-    return true;
+  return { exitCode: 0 };
 }
 
-  if (/\.plan\.md$/i.test(basename)) {
-    return true;
-  }
-
-  return false;
-}
+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);

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++);