From 64cd1ba248e77e377e76f70fc4e6434bfdddd511 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Thu, 28 May 2026 07:45:46 -0400 Subject: [PATCH] fix: surface warn-only PreToolUse hooks (#2084) --- scripts/hooks/bash-hook-dispatcher.js | 24 ++++++++++-- scripts/hooks/doc-file-warning.js | 14 +++++-- scripts/hooks/pre-bash-git-push-reminder.js | 12 ++++-- scripts/hooks/pre-bash-tmux-reminder.js | 12 ++++-- scripts/hooks/pretooluse-visible-output.js | 41 +++++++++++++++++++++ scripts/hooks/run-with-flags.js | 5 ++- tests/hooks/bash-hook-dispatcher.test.js | 11 +++++- tests/hooks/doc-file-warning.test.js | 24 ++++++++---- tests/hooks/pre-bash-reminders.test.js | 35 ++++++++++++------ 9 files changed, 143 insertions(+), 35 deletions(-) create mode 100644 scripts/hooks/pretooluse-visible-output.js diff --git a/scripts/hooks/bash-hook-dispatcher.js b/scripts/hooks/bash-hook-dispatcher.js index 9485738c..f9f0e763 100644 --- a/scripts/hooks/bash-hook-dispatcher.js +++ b/scripts/hooks/bash-hook-dispatcher.js @@ -2,6 +2,10 @@ 'use strict'; const { isHookEnabled } = require('../lib/hook-flags'); +const { + buildPreToolUseAdditionalContext, + combineAdditionalContext, +} = require('./pretooluse-visible-output'); const { run: runBlockNoVerify } = require('./block-no-verify'); const { run: runAutoTmuxDev } = require('./auto-tmux-dev'); @@ -93,7 +97,9 @@ function normalizeHookResult(previousRaw, output) { } if (output && typeof output === 'object') { - const nextRaw = Object.prototype.hasOwnProperty.call(output, 'stdout') + const nextRaw = Object.prototype.hasOwnProperty.call(output, 'additionalContext') + ? previousRaw + : Object.prototype.hasOwnProperty.call(output, 'stdout') ? String(output.stdout ?? '') : !Number.isInteger(output.exitCode) || output.exitCode === 0 ? previousRaw @@ -102,6 +108,7 @@ function normalizeHookResult(previousRaw, output) { return { raw: nextRaw, stderr: typeof output.stderr === 'string' ? output.stderr : '', + additionalContext: output.additionalContext, exitCode: Number.isInteger(output.exitCode) ? output.exitCode : 0, }; } @@ -116,6 +123,7 @@ function normalizeHookResult(previousRaw, output) { function runHooks(rawInput, hooks) { let currentRaw = rawInput; let stderr = ''; + let additionalContext = ''; for (const hook of hooks) { if (!isHookEnabled(hook.id, { profiles: hook.profiles })) { @@ -128,15 +136,25 @@ function runHooks(rawInput, hooks) { if (result.stderr) { stderr += result.stderr.endsWith('\n') ? result.stderr : `${result.stderr}\n`; } + if (result.additionalContext) { + additionalContext = combineAdditionalContext(additionalContext, result.additionalContext); + } if (result.exitCode !== 0) { - return { output: currentRaw, stderr, exitCode: result.exitCode }; + return { output: currentRaw, stderr, additionalContext, exitCode: result.exitCode }; } } catch (error) { stderr += `[Hook] ${hook.id} failed: ${error.message}\n`; } } - return { output: currentRaw, stderr, exitCode: 0 }; + return { + output: additionalContext + ? buildPreToolUseAdditionalContext(additionalContext) + : currentRaw, + stderr, + additionalContext, + exitCode: 0, + }; } function runPreBash(rawInput) { diff --git a/scripts/hooks/doc-file-warning.js b/scripts/hooks/doc-file-warning.js index a8e71262..d510b33f 100644 --- a/scripts/hooks/doc-file-warning.js +++ b/scripts/hooks/doc-file-warning.js @@ -14,6 +14,7 @@ 'use strict'; const path = require('path'); +const { buildPreToolUseAdditionalContext } = require('./pretooluse-visible-output'); const MAX_STDIN = 1024 * 1024; let data = ''; @@ -58,10 +59,11 @@ function run(inputOrRaw, _options = {}) { if (filePath && isSuspiciousDocPath(filePath)) { return { exitCode: 0, - stderr: - '[Hook] WARNING: Ad-hoc documentation filename detected\n' + - `[Hook] File: ${filePath}\n` + + additionalContext: [ + '[Hook] WARNING: Ad-hoc documentation filename detected', + `[Hook] File: ${filePath}`, '[Hook] Consider using a structured path (e.g. docs/, .claude/, skills/, .github/, benchmarks/, templates/)', + ], }; } @@ -86,5 +88,9 @@ process.stdin.on('end', () => { process.stderr.write(result.stderr + '\n'); } - process.stdout.write(data); + if (Object.prototype.hasOwnProperty.call(result, 'additionalContext')) { + process.stdout.write(buildPreToolUseAdditionalContext(result.additionalContext)); + } else { + process.stdout.write(data); + } }); diff --git a/scripts/hooks/pre-bash-git-push-reminder.js b/scripts/hooks/pre-bash-git-push-reminder.js index 62db1ff9..52ae376a 100755 --- a/scripts/hooks/pre-bash-git-push-reminder.js +++ b/scripts/hooks/pre-bash-git-push-reminder.js @@ -2,6 +2,7 @@ 'use strict'; const MAX_STDIN = 1024 * 1024; +const { buildPreToolUseAdditionalContext } = require('./pretooluse-visible-output'); let raw = ''; function run(rawInput) { @@ -10,11 +11,10 @@ function run(rawInput) { const cmd = String(input.tool_input?.command || ''); if (/\bgit\s+push\b/.test(cmd)) { return { - stdout: typeof rawInput === 'string' ? rawInput : JSON.stringify(rawInput), - stderr: [ + additionalContext: [ '[Hook] Review changes before push...', '[Hook] Continuing with push (remove this hook to add interactive review)', - ].join('\n'), + ], exitCode: 0, }; } @@ -40,7 +40,11 @@ if (require.main === module) { if (result.stderr) { process.stderr.write(`${result.stderr}\n`); } - process.stdout.write(String(result.stdout || '')); + if (Object.prototype.hasOwnProperty.call(result, 'additionalContext')) { + process.stdout.write(buildPreToolUseAdditionalContext(result.additionalContext)); + } else { + process.stdout.write(String(result.stdout || '')); + } process.exitCode = Number.isInteger(result.exitCode) ? result.exitCode : 0; return; } diff --git a/scripts/hooks/pre-bash-tmux-reminder.js b/scripts/hooks/pre-bash-tmux-reminder.js index fe3833ea..2ad56ea3 100755 --- a/scripts/hooks/pre-bash-tmux-reminder.js +++ b/scripts/hooks/pre-bash-tmux-reminder.js @@ -2,6 +2,7 @@ 'use strict'; const MAX_STDIN = 1024 * 1024; +const { buildPreToolUseAdditionalContext } = require('./pretooluse-visible-output'); let raw = ''; function run(rawInput) { @@ -15,11 +16,10 @@ function run(rawInput) { /(npm (install|test)|pnpm (install|test)|yarn (install|test)?|bun (install|test)|cargo build|make\b|docker\b|pytest|vitest|playwright)/.test(cmd) ) { return { - stdout: typeof rawInput === 'string' ? rawInput : JSON.stringify(rawInput), - stderr: [ + additionalContext: [ '[Hook] Consider running in tmux for session persistence', '[Hook] tmux new -s dev | tmux attach -t dev', - ].join('\n'), + ], exitCode: 0, }; } @@ -45,7 +45,11 @@ if (require.main === module) { if (result.stderr) { process.stderr.write(`${result.stderr}\n`); } - process.stdout.write(String(result.stdout || '')); + if (Object.prototype.hasOwnProperty.call(result, 'additionalContext')) { + process.stdout.write(buildPreToolUseAdditionalContext(result.additionalContext)); + } else { + process.stdout.write(String(result.stdout || '')); + } process.exitCode = Number.isInteger(result.exitCode) ? result.exitCode : 0; return; } diff --git a/scripts/hooks/pretooluse-visible-output.js b/scripts/hooks/pretooluse-visible-output.js new file mode 100644 index 00000000..dc481190 --- /dev/null +++ b/scripts/hooks/pretooluse-visible-output.js @@ -0,0 +1,41 @@ +#!/usr/bin/env node +'use strict'; + +function normalizeAdditionalContext(value) { + if (Array.isArray(value)) { + return value + .map(item => String(item || '').trim()) + .filter(Boolean) + .join('\n'); + } + + return String(value || '').trim(); +} + +function combineAdditionalContext(current, next) { + const currentText = normalizeAdditionalContext(current); + const nextText = normalizeAdditionalContext(next); + + if (!currentText) return nextText; + if (!nextText) return currentText; + + return `${currentText}\n${nextText}`; +} + +function buildPreToolUseAdditionalContext(value) { + const additionalContext = normalizeAdditionalContext(value); + if (!additionalContext) return ''; + + return JSON.stringify({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + additionalContext, + }, + }); +} + +module.exports = { + buildPreToolUseAdditionalContext, + combineAdditionalContext, + normalizeAdditionalContext, +}; diff --git a/scripts/hooks/run-with-flags.js b/scripts/hooks/run-with-flags.js index 84aed34a..a0b43fba 100755 --- a/scripts/hooks/run-with-flags.js +++ b/scripts/hooks/run-with-flags.js @@ -12,6 +12,7 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); const { isHookEnabled } = require('../lib/hook-flags'); +const { buildPreToolUseAdditionalContext } = require('./pretooluse-visible-output'); const MAX_STDIN = 1024 * 1024; @@ -53,7 +54,9 @@ function emitHookResult(raw, output) { if (output && typeof output === 'object') { writeStderr(output.stderr); - if (Object.prototype.hasOwnProperty.call(output, 'stdout')) { + if (Object.prototype.hasOwnProperty.call(output, 'additionalContext')) { + process.stdout.write(buildPreToolUseAdditionalContext(output.additionalContext)); + } else if (Object.prototype.hasOwnProperty.call(output, 'stdout')) { process.stdout.write(String(output.stdout ?? '')); } else if (!Number.isInteger(output.exitCode) || output.exitCode === 0) { process.stdout.write(raw); diff --git a/tests/hooks/bash-hook-dispatcher.test.js b/tests/hooks/bash-hook-dispatcher.test.js index 2f89c90b..2047462b 100644 --- a/tests/hooks/bash-hook-dispatcher.test.js +++ b/tests/hooks/bash-hook-dispatcher.test.js @@ -35,6 +35,10 @@ function runScript(scriptPath, input, env = {}) { }); } +function parseHookOutput(stdout) { + return JSON.parse(stdout); +} + function runTests() { console.log('\n=== Testing Bash hook dispatchers ===\n'); @@ -54,13 +58,18 @@ function runTests() { const enabled = runScript(preDispatcher, input, { ECC_HOOK_PROFILE: 'strict' }); assert.strictEqual(enabled.status, 0); - assert.ok(enabled.stderr.includes('Review changes before push'), 'Expected git push reminder when enabled'); + assert.strictEqual(enabled.stderr, '', `Expected visible reminder via stdout JSON, got stderr: ${enabled.stderr}`); + assert.ok( + parseHookOutput(enabled.stdout).hookSpecificOutput.additionalContext.includes('Review changes before push'), + 'Expected git push reminder when enabled' + ); const disabled = runScript(preDispatcher, input, { ECC_HOOK_PROFILE: 'strict', ECC_DISABLED_HOOKS: 'pre:bash:git-push-reminder', }); assert.strictEqual(disabled.status, 0); + assert.strictEqual(disabled.stdout, JSON.stringify(input), 'Disabled hook should pass through original input'); assert.ok(!disabled.stderr.includes('Review changes before push'), 'Disabled hook should not emit reminder'); })) passed++; else failed++; diff --git a/tests/hooks/doc-file-warning.test.js b/tests/hooks/doc-file-warning.test.js index 9d966634..2c0b3b6e 100644 --- a/tests/hooks/doc-file-warning.test.js +++ b/tests/hooks/doc-file-warning.test.js @@ -28,6 +28,10 @@ function runScript(input) { return { code: result.status || 0, stdout: result.stdout || '', stderr: result.stderr || '' }; } +function parseHookOutput(stdout) { + return JSON.parse(stdout); +} + function runTests() { console.log('\n=== Testing doc-file-warning.js (denylist policy) ===\n'); let passed = 0; @@ -138,10 +142,13 @@ function runTests() { ]; for (const file of deniedFiles) { (test(`warns on ad-hoc denylist file: ${file}`, () => { - const { code, stderr } = runScript({ tool_input: { file_path: file } }); + const { code, stdout, 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}`); - assert.ok(stderr.includes(file), `expected file path in stderr for ${file}`); + assert.strictEqual(stderr, '', `expected visible warning via stdout JSON, got stderr: ${stderr}`); + const output = parseHookOutput(stdout); + const additionalContext = output.hookSpecificOutput?.additionalContext || ''; + assert.ok(additionalContext.includes('WARNING'), `expected warning in additionalContext for ${file}, got: ${stdout}`); + assert.ok(additionalContext.includes(file), `expected file path in additionalContext for ${file}`); }) ? passed++ : failed++); } @@ -153,9 +160,10 @@ function runTests() { }) ? 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' } }); + const { code, stdout, 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'); + assert.strictEqual(stderr, '', `expected visible warning via stdout JSON, got stderr: ${stderr}`); + assert.ok(parseHookOutput(stdout).hookSpecificOutput.additionalContext.includes('WARNING'), 'expected warning for non-structured backslash path'); }) ? passed++ : failed++); // 8. Invalid/empty input - passes through without error @@ -196,10 +204,12 @@ function runTests() { assert.strictEqual(stdout, JSON.stringify(input)); }) ? passed++ : failed++); - (test('passes through input to stdout for warned file', () => { + (test('emits visible additionalContext JSON for warned file', () => { const input = { tool_input: { file_path: 'TODO.md' } }; const { stdout } = runScript(input); - assert.strictEqual(stdout, JSON.stringify(input)); + const output = parseHookOutput(stdout); + assert.strictEqual(output.hookSpecificOutput.hookEventName, 'PreToolUse'); + assert.ok(output.hookSpecificOutput.additionalContext.includes('TODO.md')); }) ? passed++ : failed++); (test('passes through input to stdout for empty input', () => { diff --git a/tests/hooks/pre-bash-reminders.test.js b/tests/hooks/pre-bash-reminders.test.js index 507059a2..def6be6c 100644 --- a/tests/hooks/pre-bash-reminders.test.js +++ b/tests/hooks/pre-bash-reminders.test.js @@ -35,6 +35,10 @@ function runScript(scriptPath, command, envOverrides = {}) { return { code: result.status || 0, stdout: result.stdout || '', stderr: result.stderr || '', inputStr }; } +function parseHookOutput(stdout) { + return JSON.parse(stdout); +} + function runTests() { console.log('\n=== Testing pre-bash-git-push-reminder.js & pre-bash-tmux-reminder.js ===\n'); @@ -45,11 +49,13 @@ function runTests() { console.log(' git-push-reminder:'); - (test('git push triggers stderr warning', () => { + (test('git push triggers visible additionalContext warning', () => { const result = runScript(gitPushScript, 'git push origin main'); assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`); - assert.ok(result.stderr.includes('[Hook]'), `Expected stderr to contain [Hook], got: ${result.stderr}`); - assert.ok(result.stderr.includes('Review changes before push'), `Expected stderr to mention review`); + assert.strictEqual(result.stderr, '', `Expected no stderr, got: ${result.stderr}`); + const additionalContext = parseHookOutput(result.stdout).hookSpecificOutput.additionalContext; + assert.ok(additionalContext.includes('[Hook]'), `Expected additionalContext to contain [Hook], got: ${result.stdout}`); + assert.ok(additionalContext.includes('Review changes before push'), `Expected additionalContext to mention review`); }) ? passed++ : failed++); (test('git status has no warning', () => { @@ -58,9 +64,11 @@ function runTests() { assert.strictEqual(result.stderr, '', `Expected no stderr, got: ${result.stderr}`); }) ? passed++ : failed++); - (test('git push always passes through input on stdout', () => { + (test('git push emits PreToolUse additionalContext JSON on stdout', () => { const result = runScript(gitPushScript, 'git push'); - assert.strictEqual(result.stdout, result.inputStr, 'Expected stdout to match original input'); + const output = parseHookOutput(result.stdout); + assert.strictEqual(output.hookSpecificOutput.hookEventName, 'PreToolUse'); + assert.ok(output.hookSpecificOutput.additionalContext.includes('Review changes before push')); }) ? passed++ : failed++); // --- tmux-reminder tests (non-Windows only) --- @@ -70,17 +78,20 @@ function runTests() { if (!isWindows) { console.log('\n tmux-reminder:'); - (test('npm install triggers tmux suggestion', () => { + (test('npm install triggers visible tmux suggestion', () => { const result = runScript(tmuxScript, 'npm install', { TMUX: '' }); assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`); - assert.ok(result.stderr.includes('[Hook]'), `Expected stderr to contain [Hook], got: ${result.stderr}`); - assert.ok(result.stderr.includes('tmux'), `Expected stderr to mention tmux`); + assert.strictEqual(result.stderr, '', `Expected no stderr, got: ${result.stderr}`); + const additionalContext = parseHookOutput(result.stdout).hookSpecificOutput.additionalContext; + assert.ok(additionalContext.includes('[Hook]'), `Expected additionalContext to contain [Hook], got: ${result.stdout}`); + assert.ok(additionalContext.includes('tmux'), `Expected additionalContext to mention tmux`); }) ? passed++ : failed++); (test('npm test triggers tmux suggestion', () => { const result = runScript(tmuxScript, 'npm test', { TMUX: '' }); assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`); - assert.ok(result.stderr.includes('tmux'), `Expected stderr to mention tmux`); + assert.strictEqual(result.stderr, '', `Expected no stderr, got: ${result.stderr}`); + assert.ok(parseHookOutput(result.stdout).hookSpecificOutput.additionalContext.includes('tmux'), `Expected additionalContext to mention tmux`); }) ? passed++ : failed++); (test('regular command like ls has no tmux suggestion', () => { @@ -89,9 +100,11 @@ function runTests() { assert.strictEqual(result.stderr, '', `Expected no stderr for ls, got: ${result.stderr}`); }) ? passed++ : failed++); - (test('tmux reminder always passes through input on stdout', () => { + (test('tmux reminder emits PreToolUse additionalContext JSON on stdout', () => { const result = runScript(tmuxScript, 'npm install', { TMUX: '' }); - assert.strictEqual(result.stdout, result.inputStr, 'Expected stdout to match original input'); + const output = parseHookOutput(result.stdout); + assert.strictEqual(output.hookSpecificOutput.hookEventName, 'PreToolUse'); + assert.ok(output.hookSpecificOutput.additionalContext.includes('tmux')); }) ? passed++ : failed++); } else { console.log('\n (skipping tmux-reminder tests on Windows)\n');