diff --git a/scripts/hooks/doc-file-warning.js b/scripts/hooks/doc-file-warning.js index d510b33f..40d0282a 100644 --- a/scripts/hooks/doc-file-warning.js +++ b/scripts/hooks/doc-file-warning.js @@ -17,7 +17,6 @@ const path = require('path'); const { buildPreToolUseAdditionalContext } = require('./pretooluse-visible-output'); const MAX_STDIN = 1024 * 1024; -let data = ''; // 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)$/; @@ -70,27 +69,40 @@ function run(inputOrRaw, _options = {}) { return { exitCode: 0 }; } -module.exports = { run }; +/** + * Stdin entrypoint for direct/spawnSync execution: reads the hook payload from + * stdin (capped at MAX_STDIN), runs the policy, and writes the PreToolUse result + * to stdout. Must only run when invoked directly, never on require(), so the + * stdin listeners are not leaked into a parent that loads this hook in-process. + */ +function main() { + let data = ''; + process.stdin.setEncoding('utf8'); + process.stdin.on('data', c => { + if (data.length < MAX_STDIN) { + const remaining = MAX_STDIN - data.length; + data += c.substring(0, remaining); + } + }); -// Stdin fallback for spawnSync execution -process.stdin.setEncoding('utf8'); -process.stdin.on('data', c => { - if (data.length < MAX_STDIN) { - const remaining = MAX_STDIN - data.length; - data += c.substring(0, remaining); - } -}); + process.stdin.on('end', () => { + const result = run(data); -process.stdin.on('end', () => { - const result = run(data); + if (result.stderr) { + process.stderr.write(result.stderr + '\n'); + } - if (result.stderr) { - process.stderr.write(result.stderr + '\n'); - } + if (Object.prototype.hasOwnProperty.call(result, 'additionalContext')) { + process.stdout.write(buildPreToolUseAdditionalContext(result.additionalContext)); + } else { + process.stdout.write(data); + } + }); +} - if (Object.prototype.hasOwnProperty.call(result, 'additionalContext')) { - process.stdout.write(buildPreToolUseAdditionalContext(result.additionalContext)); - } else { - process.stdout.write(data); - } -}); +module.exports = { run, main }; + +// Stdin fallback for spawnSync execution — only when invoked directly, not via require() +if (require.main === module) { + main(); +} diff --git a/scripts/hooks/pre-write-doc-warn.js b/scripts/hooks/pre-write-doc-warn.js index ca515111..856b5159 100644 --- a/scripts/hooks/pre-write-doc-warn.js +++ b/scripts/hooks/pre-write-doc-warn.js @@ -6,4 +6,5 @@ 'use strict'; -require('./doc-file-warning.js'); +// doc-file-warning.js guards its stdin entrypoint behind require.main; call main() explicitly. +require('./doc-file-warning.js').main(); diff --git a/tests/hooks/doc-file-warning.test.js b/tests/hooks/doc-file-warning.test.js index 2c0b3b6e..fd241132 100644 --- a/tests/hooks/doc-file-warning.test.js +++ b/tests/hooks/doc-file-warning.test.js @@ -218,6 +218,47 @@ function runTests() { assert.strictEqual(stdout, JSON.stringify(input)); }) ? passed++ : failed++); + // 11. Regression: requiring the hook in-process (run-with-flags fast path) must not + // attach module-scope stdin listeners to the dispatcher process. + (test('require() does not attach stdin listeners (in-process safe)', () => { + const resolved = require.resolve(script); + delete require.cache[resolved]; + const endBefore = process.stdin.listenerCount('end'); + const dataBefore = process.stdin.listenerCount('data'); + const mod = require(resolved); + assert.strictEqual(process.stdin.listenerCount('end'), endBefore, + 'require() must not attach a stdin "end" listener'); + assert.strictEqual(process.stdin.listenerCount('data'), dataBefore, + 'require() must not attach a stdin "data" listener'); + assert.strictEqual(typeof mod.run, 'function', 'run() must remain exported'); + assert.strictEqual(typeof mod.main, 'function', 'main() must be exported for entrypoints'); + }) ? passed++ : failed++); + + // 12. Regression: exported run() still classifies correctly in-process, no stdin needed. + (test('exported run() works in-process for warned and allowed files', () => { + delete require.cache[require.resolve(script)]; + const { run } = require(script); + const warned = run(JSON.stringify({ tool_input: { file_path: 'TODO.md' } })); + assert.ok(Array.isArray(warned.additionalContext) + && warned.additionalContext.join('\n').includes('WARNING'), + 'warned file should return additionalContext with WARNING'); + const allowed = run(JSON.stringify({ tool_input: { file_path: 'README.md' } })); + assert.ok(!('additionalContext' in allowed), 'allowed file should not return additionalContext'); + }) ? passed++ : failed++); + + // 13. Regression: pre-write-doc-warn.js backward-compat entrypoint still emits the warning. + (test('pre-write-doc-warn.js shim still warns via main()', () => { + const shim = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'pre-write-doc-warn.js'); + const r = spawnSync('node', [shim], { + encoding: 'utf8', + input: JSON.stringify({ tool_input: { file_path: 'TODO.md' } }), + timeout: 10000, + }); + assert.strictEqual(r.status || 0, 0, 'shim should exit 0'); + assert.ok(JSON.parse(r.stdout).hookSpecificOutput.additionalContext.includes('TODO.md'), + 'shim should still emit the ad-hoc filename warning'); + }) ? passed++ : failed++); + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); }