From 8c86836fbf61b7bbd6c5a5625d4c1a670e123ddf Mon Sep 17 00:00:00 2001 From: Jamkris Date: Thu, 23 Apr 2026 12:40:15 +0900 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20make=20hook=20commands=20cross-platf?= =?UTF-8?q?orm=20(Windows=20PowerShell)=20=E2=80=94=20fixes=20#42?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #42 reports "Alerta constante" parser errors on Windows, caused by POSIX-only shell syntax in hooks/hooks.json: node "$HOME/.../scripts/hooks/pre-compact.js" 2>/dev/null \ || node "$HOME/.gemini/scripts/hooks/pre-compact.js" 2>/dev/null \ || true Windows PowerShell 5.x (the default shell on Windows 10/11) rejects `||` as an "invalid statement separator" and has no `/dev/null` or `true`. Replace the six script-launching hook commands with a single Node launcher: node "$HOME/.../scripts/hooks/run.js" The launcher (scripts/hooks/run.js) resolves the target hook file via `__dirname`, silently skips missing hooks (preserving the prior `|| true` semantics), and never fails the parent Gemini CLI action. Inline `node -e "..."` hooks are untouched — the JS string is consumed by Node, not the shell, so they are already cross-platform. Tests: - Add a regression guard asserting no hook command contains ` || `, `2>/dev/null`, or ` || true` (inline `node -e` commands exempted). - Add a sanity check on run.js for __dirname + argv[2] usage. --- hooks/hooks.json | 12 +++++------ scripts/hooks/run.js | 41 ++++++++++++++++++++++++++++++++++++ tests/hooks/hooks.test.js | 44 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 6 deletions(-) create mode 100755 scripts/hooks/run.js diff --git a/hooks/hooks.json b/hooks/hooks.json index d64af23..f0d79af 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -47,7 +47,7 @@ "hooks": [ { "type": "command", - "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/suggest-compact.js\" 2>/dev/null || node \"$HOME/.gemini/scripts/hooks/suggest-compact.js\" 2>/dev/null || true" + "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/run.js\" suggest-compact" } ], "description": "Suggest manual compaction at logical intervals" @@ -59,7 +59,7 @@ "hooks": [ { "type": "command", - "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/pre-compact.js\" 2>/dev/null || node \"$HOME/.gemini/scripts/hooks/pre-compact.js\" 2>/dev/null || true" + "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/run.js\" pre-compact" } ], "description": "Run pre-compression logic" @@ -71,7 +71,7 @@ "hooks": [ { "type": "command", - "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/session-start.js\" 2>/dev/null || node \"$HOME/.gemini/scripts/hooks/session-start.js\" 2>/dev/null || true" + "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/run.js\" session-start" } ], "description": "Load previous context and detect package manager on new session" @@ -137,7 +137,7 @@ "hooks": [ { "type": "command", - "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/check-console-log.js\" 2>/dev/null || node \"$HOME/.gemini/scripts/hooks/check-console-log.js\" 2>/dev/null || true" + "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/run.js\" check-console-log" } ], "description": "Check for console.log in modified files after each response" @@ -149,7 +149,7 @@ "hooks": [ { "type": "command", - "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/session-end.js\" 2>/dev/null || node \"$HOME/.gemini/scripts/hooks/session-end.js\" 2>/dev/null || true" + "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/run.js\" session-end" } ], "description": "Persist session state on end" @@ -159,7 +159,7 @@ "hooks": [ { "type": "command", - "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/evaluate-session.js\" 2>/dev/null || node \"$HOME/.gemini/scripts/hooks/evaluate-session.js\" 2>/dev/null || true" + "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/run.js\" evaluate-session" } ], "description": "Evaluate session for extractable patterns" diff --git a/scripts/hooks/run.js b/scripts/hooks/run.js new file mode 100755 index 0000000..fd70c14 --- /dev/null +++ b/scripts/hooks/run.js @@ -0,0 +1,41 @@ +#!/usr/bin/env node +/** + * Cross-platform hook launcher. + * + * Invoked from hooks.json as: + * node "/scripts/hooks/run.js" + * + * Resolves the target hook file relative to this launcher (__dirname) so the + * same hooks.json works regardless of whether the extension was installed to + * ~/.gemini/extensions/... or copied under ~/.gemini/ directly. Avoids + * POSIX-only shell constructs (`||`, `2>/dev/null`, `true`) that break on + * Windows PowerShell 5.x. See issue #42. + */ + +'use strict'; + +const path = require('node:path'); +const fs = require('node:fs'); +const { spawnSync } = require('node:child_process'); + +const hookName = process.argv[2]; +if (!hookName || !/^[a-z][a-z0-9-]*$/i.test(hookName)) { + // No hook name, or a suspicious value — exit silently. + process.exit(0); +} + +const hookPath = path.join(__dirname, `${hookName}.js`); +if (!fs.existsSync(hookPath)) { + // Optional hook not installed — stay quiet, matches the prior `|| true` behavior. + process.exit(0); +} + +// Run the hook in its own Node process so it inherits stdin/stdout/stderr +// unchanged (hooks read JSON payloads on stdin and may write to stdout/stderr). +// Mirror the previous "fail soft" semantics: a hook error never fails the +// parent Gemini CLI action. Surface via GEMINI_HOOK_DEBUG=1 when diagnosing. +const result = spawnSync(process.execPath, [hookPath], { stdio: 'inherit' }); +if (result.error && process.env.GEMINI_HOOK_DEBUG) { + console.error(`[hook:${hookName}] spawn error:`, result.error); +} +process.exit(0); diff --git a/tests/hooks/hooks.test.js b/tests/hooks/hooks.test.js index 0a4b0eb..ef6552e 100644 --- a/tests/hooks/hooks.test.js +++ b/tests/hooks/hooks.test.js @@ -351,6 +351,50 @@ async function runTests() { } })) passed++; else failed++; + if (test('hook script commands avoid POSIX-only shell syntax (Windows PS compat)', () => { + // Regression guard for #42. Hook commands that invoke a script file must + // not chain with `||`, redirect with `2>/dev/null`, or rely on `true` — + // Windows PowerShell 5.x parses `||` as an error and has no /dev/null. + // Inline `node -e "..."` payloads are exempt because the JS string is + // consumed by Node, not the shell. + const hooksPath = path.join(__dirname, '..', '..', 'hooks', 'hooks.json'); + const hooks = JSON.parse(fs.readFileSync(hooksPath, 'utf8')); + const hooksObj = hooks.hooks || hooks; + + const checkHooks = (hookArray) => { + if (!hookArray) return; + for (const entry of hookArray) { + for (const hook of entry.hooks) { + if (hook.type !== 'command') continue; + if (hook.command.startsWith('node -e ')) continue; + const bad = [' || ', '2>/dev/null', ' || true']; + for (const pattern of bad) { + assert.ok( + !hook.command.includes(pattern), + `Hook command must not use POSIX-only "${pattern}" (breaks on Windows PowerShell): ${hook.command.substring(0, 80)}...` + ); + } + } + } + }; + + if (Array.isArray(hooksObj)) { + checkHooks(hooksObj); + } else { + for (const [, hookArray] of Object.entries(hooksObj)) { + checkHooks(hookArray); + } + } + })) passed++; else failed++; + + if (test('run.js launcher resolves hook files from its own directory', () => { + const launcher = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'run.js'); + assert.ok(fs.existsSync(launcher), 'scripts/hooks/run.js must exist'); + const src = fs.readFileSync(launcher, 'utf8'); + assert.ok(src.includes('__dirname'), 'launcher should resolve hooks relative to __dirname'); + assert.ok(src.includes('process.argv[2]'), 'launcher should read hook name from argv[2]'); + })) passed++; else failed++; + // plugin.json validation console.log('\nplugin.json Validation:'); From 6644ade5469f8c867085d45b8a472d2ada8c6d63 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Thu, 23 Apr 2026 12:48:53 +0900 Subject: [PATCH 2/2] fix: bound hook runner timeout and harden regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit feedback on PR #43. - scripts/hooks/run.js: bound hook execution with a 30s default timeout via spawnSync, configurable through GEMINI_HOOK_TIMEOUT_MS (set to 0 to disable). Prevents a hung hook from stalling a Gemini session while preserving fail-soft semantics. - tests/hooks/hooks.test.js: replace substring checks with regexes so the POSIX-syntax guard also catches variants like `||true`, `cmd|| true`, and `2> /dev/null`. Strip comments before checking the launcher implementation so the header docblock can't satisfy the __dirname/argv[2] assertions; assert on `path.join(__dirname` instead. Not applied: CodeRabbit's "restore manual-install fallback" suggestion. scripts/install.sh only copies scripts/* to ~/.gemini/scripts/ — it does NOT copy hooks/hooks.json. In manual install mode the file is never loaded (confirmed by the existing "Gemini CLI automatically loads hooks/hooks.json by convention" note in tests), so the removed fallback path was dead code, not a supported surface. Rebuttal posted inline. --- scripts/hooks/run.js | 14 +++++++++++++- tests/hooks/hooks.test.js | 27 +++++++++++++++++++++------ 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/scripts/hooks/run.js b/scripts/hooks/run.js index fd70c14..52885c2 100755 --- a/scripts/hooks/run.js +++ b/scripts/hooks/run.js @@ -34,7 +34,19 @@ if (!fs.existsSync(hookPath)) { // unchanged (hooks read JSON payloads on stdin and may write to stdout/stderr). // Mirror the previous "fail soft" semantics: a hook error never fails the // parent Gemini CLI action. Surface via GEMINI_HOOK_DEBUG=1 when diagnosing. -const result = spawnSync(process.execPath, [hookPath], { stdio: 'inherit' }); +// Bound execution time so a hung hook can't stall a Gemini session. Override +// via GEMINI_HOOK_TIMEOUT_MS (e.g. set to a larger value for slow hooks, or +// to 0 to disable the bound). +const DEFAULT_HOOK_TIMEOUT_MS = 30_000; +const parsedTimeout = Number.parseInt(process.env.GEMINI_HOOK_TIMEOUT_MS || '', 10); +const timeout = Number.isFinite(parsedTimeout) && parsedTimeout >= 0 + ? parsedTimeout + : DEFAULT_HOOK_TIMEOUT_MS; + +const spawnOptions = { stdio: 'inherit' }; +if (timeout > 0) spawnOptions.timeout = timeout; + +const result = spawnSync(process.execPath, [hookPath], spawnOptions); if (result.error && process.env.GEMINI_HOOK_DEBUG) { console.error(`[hook:${hookName}] spawn error:`, result.error); } diff --git a/tests/hooks/hooks.test.js b/tests/hooks/hooks.test.js index ef6552e..fae7f4e 100644 --- a/tests/hooks/hooks.test.js +++ b/tests/hooks/hooks.test.js @@ -367,11 +367,16 @@ async function runTests() { for (const hook of entry.hooks) { if (hook.type !== 'command') continue; if (hook.command.startsWith('node -e ')) continue; - const bad = [' || ', '2>/dev/null', ' || true']; - for (const pattern of bad) { + // Regex guards catch variants like `||true`, `cmd|| true`, `2> /dev/null`. + const bad = [ + { pattern: /\|\|/, label: '||' }, + { pattern: /2>\s*\/dev\/null\b/, label: '2>/dev/null' }, + { pattern: /(^|\s)true(\s|$)/, label: 'trailing `true`' }, + ]; + for (const { pattern, label } of bad) { assert.ok( - !hook.command.includes(pattern), - `Hook command must not use POSIX-only "${pattern}" (breaks on Windows PowerShell): ${hook.command.substring(0, 80)}...` + !pattern.test(hook.command), + `Hook command must not use POSIX-only "${label}" (breaks on Windows PowerShell): ${hook.command.substring(0, 80)}...` ); } } @@ -391,8 +396,18 @@ async function runTests() { const launcher = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'run.js'); assert.ok(fs.existsSync(launcher), 'scripts/hooks/run.js must exist'); const src = fs.readFileSync(launcher, 'utf8'); - assert.ok(src.includes('__dirname'), 'launcher should resolve hooks relative to __dirname'); - assert.ok(src.includes('process.argv[2]'), 'launcher should read hook name from argv[2]'); + // Strip block + line comments so header doc can't satisfy the assertions. + const code = src + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^\s*\/\/.*$/gm, ''); + assert.ok( + /path\.join\(\s*__dirname/.test(code), + 'launcher should resolve hooks via path.join(__dirname, ...)' + ); + assert.ok( + /process\.argv\[2\]/.test(code), + 'launcher should read hook name from process.argv[2]' + ); })) passed++; else failed++; // plugin.json validation