From 8a2d13187cfa20ba470ffc6d4d6f6429d3ebe911 Mon Sep 17 00:00:00 2001 From: seto Date: Sun, 12 Apr 2026 17:42:32 +0900 Subject: [PATCH] fix: address P1 review feedback from greptile bot 1. Use run-with-flags.js wrapper (supports ECC_HOOK_PROFILE, ECC_DISABLED_HOOKS) 2. Add session timeout (30min inactivity = state reset, fixes "once ever" bug) 3. Add 9 integration tests (deny/allow/timeout/sanitize/disable) Refactored hook to module.exports.run() pattern for direct require() by run-with-flags.js (~50-100ms faster per invocation). Co-Authored-By: Claude Opus 4.6 --- hooks/hooks.json | 4 +- scripts/hooks/gateguard-fact-force.js | 93 ++++---- tests/hooks/gateguard-fact-force.test.js | 261 +++++++++++++++++++++++ 3 files changed, 302 insertions(+), 56 deletions(-) create mode 100644 tests/hooks/gateguard-fact-force.test.js diff --git a/hooks/hooks.json b/hooks/hooks.json index 0dc92970..13224dcb 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -132,7 +132,7 @@ "hooks": [ { "type": "command", - "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/gateguard-fact-force.js\"", + "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/run-with-flags.js\" \"pre:edit-write:gateguard-fact-force\" \"scripts/hooks/gateguard-fact-force.js\" \"standard,strict\"", "timeout": 5 } ], @@ -144,7 +144,7 @@ "hooks": [ { "type": "command", - "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/gateguard-fact-force.js\"", + "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/run-with-flags.js\" \"pre:bash:gateguard-fact-force\" \"scripts/hooks/gateguard-fact-force.js\" \"standard,strict\"", "timeout": 5 } ], diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index d1e0106b..2cf3d433 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -13,10 +13,7 @@ * - Bash (destructive): list targets, rollback plan, quote instruction * - Bash (routine): quote current instruction (once per session) * - * Exit codes: - * 0 - Allow (gate already passed for this target) - * 2 - Block (force investigation first) - * + * Compatible with run-with-flags.js via module.exports.run(). * Cross-platform (Windows, macOS, Linux). * * Full package with config support: pip install gateguard-ai @@ -28,27 +25,35 @@ const fs = require('fs'); const path = require('path'); -const MAX_STDIN = 1024 * 1024; - // Session state file for tracking which files have been gated const STATE_DIR = path.join(process.env.HOME || process.env.USERPROFILE || '/tmp', '.gateguard'); const STATE_FILE = path.join(STATE_DIR, '.session_state.json'); +// State expires after 30 minutes of inactivity (= new session) +const SESSION_TIMEOUT_MS = 30 * 60 * 1000; + const DESTRUCTIVE_BASH = /\b(rm\s+-rf|git\s+reset\s+--hard|git\s+checkout\s+--|git\s+clean\s+-f|drop\s+table|delete\s+from|truncate|git\s+push\s+--force|dd\s+if=)\b/i; -// --- State management --- +// --- State management (with session timeout) --- function loadState() { try { if (fs.existsSync(STATE_FILE)) { - return JSON.parse(fs.readFileSync(STATE_FILE, 'utf8')); + const state = JSON.parse(fs.readFileSync(STATE_FILE, 'utf8')); + const lastActive = state.last_active || 0; + if (Date.now() - lastActive > SESSION_TIMEOUT_MS) { + // Session expired — start fresh + return { checked: [], last_active: Date.now() }; + } + return state; } } catch (_) { /* ignore */ } - return { checked: [], read_files: [] }; + return { checked: [], last_active: Date.now() }; } function saveState(state) { try { + state.last_active = Date.now(); fs.mkdirSync(STATE_DIR, { recursive: true }); fs.writeFileSync(STATE_FILE, JSON.stringify(state, null, 2), 'utf8'); } catch (_) { /* ignore */ } @@ -64,6 +69,8 @@ function markChecked(key) { function isChecked(key) { const state = loadState(); + // Touch last_active on every check + saveState(state); return state.checked.includes(key); } @@ -130,42 +137,29 @@ function routineBashMsg() { ].join('\n'); } -// --- Output helpers --- +// --- Deny helper --- -function deny(reason) { - const output = { - hookSpecificOutput: { - hookEventName: 'PreToolUse', - permissionDecision: 'deny', - permissionDecisionReason: reason - } +function denyResult(reason) { + return { + stdout: JSON.stringify({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + permissionDecision: 'deny', + permissionDecisionReason: reason + } + }), + exitCode: 0 }; - process.stdout.write(JSON.stringify(output)); - process.exit(0); } -function allow() { - // Output nothing = allow - process.exit(0); -} - -// --- Main --- - -function main() { - let raw = ''; - try { - raw = fs.readFileSync(0, 'utf8').slice(0, MAX_STDIN); - } catch (_) { - allow(); - return; - } +// --- Core logic (exported for run-with-flags.js) --- +function run(rawInput) { let data; try { - data = JSON.parse(raw); + data = typeof rawInput === 'string' ? JSON.parse(rawInput) : rawInput; } catch (_) { - allow(); - return; + return rawInput; // allow on parse error } const toolName = data.tool_name || ''; @@ -174,43 +168,34 @@ function main() { if (toolName === 'Edit' || toolName === 'Write') { const filePath = toolInput.file_path || ''; if (!filePath) { - allow(); - return; + return rawInput; // allow } - // Gate: first action per file if (!isChecked(filePath)) { markChecked(filePath); const msg = toolName === 'Edit' ? editGateMsg(filePath) : writeGateMsg(filePath); - deny(msg); - return; + return denyResult(msg); } - allow(); - return; + return rawInput; // allow } if (toolName === 'Bash') { const command = toolInput.command || ''; - // Destructive commands: always gate if (DESTRUCTIVE_BASH.test(command)) { - deny(destructiveBashMsg()); - return; + return denyResult(destructiveBashMsg()); } - // Routine bash: once per session if (!isChecked('__bash_session__')) { markChecked('__bash_session__'); - deny(routineBashMsg()); - return; + return denyResult(routineBashMsg()); } - allow(); - return; + return rawInput; // allow } - allow(); + return rawInput; // allow } -main(); +module.exports = { run }; diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js new file mode 100644 index 00000000..de8dfd6a --- /dev/null +++ b/tests/hooks/gateguard-fact-force.test.js @@ -0,0 +1,261 @@ +/** + * Tests for scripts/hooks/gateguard-fact-force.js via run-with-flags.js + */ + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const runner = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'run-with-flags.js'); +const stateDir = path.join(process.env.HOME || process.env.USERPROFILE || '/tmp', '.gateguard'); +const stateFile = path.join(stateDir, '.session_state.json'); + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + return true; + } catch (error) { + console.log(` ✗ ${name}`); + console.log(` Error: ${error.message}`); + return false; + } +} + +function clearState() { + try { + if (fs.existsSync(stateFile)) { + fs.unlinkSync(stateFile); + } + } catch (_) { /* ignore */ } +} + +function writeExpiredState() { + try { + fs.mkdirSync(stateDir, { recursive: true }); + const expired = { + checked: ['some_file.js', '__bash_session__'], + last_active: Date.now() - (31 * 60 * 1000) // 31 minutes ago + }; + fs.writeFileSync(stateFile, JSON.stringify(expired), 'utf8'); + } catch (_) { /* ignore */ } +} + +function runHook(input, env = {}) { + const rawInput = typeof input === 'string' ? input : JSON.stringify(input); + const result = spawnSync('node', [ + runner, + 'pre:edit-write:gateguard-fact-force', + 'scripts/hooks/gateguard-fact-force.js', + 'standard,strict' + ], { + input: rawInput, + encoding: 'utf8', + env: { + ...process.env, + ECC_HOOK_PROFILE: 'standard', + ...env + }, + timeout: 15000, + stdio: ['pipe', 'pipe', 'pipe'] + }); + + return { + code: Number.isInteger(result.status) ? result.status : 1, + stdout: result.stdout || '', + stderr: result.stderr || '' + }; +} + +function runBashHook(input, env = {}) { + const rawInput = typeof input === 'string' ? input : JSON.stringify(input); + const result = spawnSync('node', [ + runner, + 'pre:bash:gateguard-fact-force', + 'scripts/hooks/gateguard-fact-force.js', + 'standard,strict' + ], { + input: rawInput, + encoding: 'utf8', + env: { + ...process.env, + ECC_HOOK_PROFILE: 'standard', + ...env + }, + timeout: 15000, + stdio: ['pipe', 'pipe', 'pipe'] + }); + + return { + code: Number.isInteger(result.status) ? result.status : 1, + stdout: result.stdout || '', + stderr: result.stderr || '' + }; +} + +function parseOutput(stdout) { + try { + return JSON.parse(stdout); + } catch (_) { + return null; + } +} + +function runTests() { + console.log('\n=== Testing gateguard-fact-force ===\n'); + + let passed = 0; + let failed = 0; + + // --- Test 1: denies first Edit per file --- + clearState(); + if (test('denies first Edit per file with fact-forcing message', () => { + const input = { + tool_name: 'Edit', + tool_input: { file_path: '/src/app.js', old_string: 'foo', new_string: 'bar' } + }; + const result = runHook(input); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON output'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny'); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('Fact-Forcing Gate')); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('import/require')); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('/src/app.js')); + })) passed++; else failed++; + + // --- Test 2: allows second Edit on same file --- + if (test('allows second Edit on same file (gate already passed)', () => { + const input = { + tool_name: 'Edit', + tool_input: { file_path: '/src/app.js', old_string: 'foo', new_string: 'bar' } + }; + const result = runHook(input); + const output = parseOutput(result.stdout); + if (output && output.hookSpecificOutput) { + assert.notStrictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'should not deny second edit on same file'); + } + })) passed++; else failed++; + + // --- Test 3: denies first Write per file --- + clearState(); + if (test('denies first Write per file with fact-forcing message', () => { + const input = { + tool_name: 'Write', + tool_input: { file_path: '/src/new-file.js', content: 'console.log("hello")' } + }; + const result = runHook(input); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON output'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny'); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('creating')); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('call this new file')); + })) passed++; else failed++; + + // --- Test 4: denies destructive Bash --- + clearState(); + if (test('denies destructive Bash commands', () => { + const input = { + tool_name: 'Bash', + tool_input: { command: 'rm -rf /important/data' } + }; + const result = runBashHook(input); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON output'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny'); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('Destructive')); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('rollback')); + })) passed++; else failed++; + + // --- Test 5: denies first routine Bash, allows second --- + clearState(); + if (test('denies first routine Bash, allows second', () => { + const input = { + tool_name: 'Bash', + tool_input: { command: 'ls -la' } + }; + + // First call: should deny + const result1 = runBashHook(input); + const output1 = parseOutput(result1.stdout); + assert.ok(output1, 'first call should produce JSON output'); + assert.strictEqual(output1.hookSpecificOutput.permissionDecision, 'deny'); + + // Second call: should allow + const result2 = runBashHook(input); + const output2 = parseOutput(result2.stdout); + if (output2 && output2.hookSpecificOutput) { + assert.notStrictEqual(output2.hookSpecificOutput.permissionDecision, 'deny'); + } + })) passed++; else failed++; + + // --- Test 6: session state resets after timeout --- + if (test('session state resets after 30-minute timeout', () => { + writeExpiredState(); + const input = { + tool_name: 'Edit', + tool_input: { file_path: 'some_file.js', old_string: 'a', new_string: 'b' } + }; + const result = runHook(input); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON output after expired state'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'should deny again after session timeout (state was reset)'); + })) passed++; else failed++; + + // --- Test 7: allows unknown tool names --- + clearState(); + if (test('allows unknown tool names through', () => { + const input = { + tool_name: 'Read', + tool_input: { file_path: '/src/app.js' } + }; + const result = runHook(input); + const output = parseOutput(result.stdout); + if (output && output.hookSpecificOutput) { + assert.notStrictEqual(output.hookSpecificOutput.permissionDecision, 'deny'); + } + })) passed++; else failed++; + + // --- Test 8: sanitizes file paths with newlines --- + clearState(); + if (test('sanitizes file paths containing newlines', () => { + const input = { + tool_name: 'Edit', + tool_input: { file_path: '/src/app.js\ninjected content', old_string: 'a', new_string: 'b' } + }; + const result = runHook(input); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON output'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny'); + const reason = output.hookSpecificOutput.permissionDecisionReason; + assert.ok(!reason.includes('injected content\n'), 'newline injection should be sanitized'); + })) passed++; else failed++; + + // --- Test 9: respects ECC_DISABLED_HOOKS --- + clearState(); + if (test('respects ECC_DISABLED_HOOKS (skips when disabled)', () => { + const input = { + tool_name: 'Edit', + tool_input: { file_path: '/src/disabled.js', old_string: 'a', new_string: 'b' } + }; + const result = runHook(input, { + ECC_DISABLED_HOOKS: 'pre:edit-write:gateguard-fact-force' + }); + + const output = parseOutput(result.stdout); + if (output && output.hookSpecificOutput) { + assert.notStrictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'should not deny when hook is disabled'); + } + })) passed++; else failed++; + + // Cleanup + clearState(); + + console.log(`\n ${passed} passed, ${failed} failed\n`); + process.exit(failed > 0 ? 1 : 0); +} + +runTests();