Skip to content

fix: make hook commands cross-platform (Windows PowerShell) — fixes #42#43

Merged
Jamkris merged 2 commits intomainfrom
fix/windows-hook-compat
Apr 23, 2026
Merged

fix: make hook commands cross-platform (Windows PowerShell) — fixes #42#43
Jamkris merged 2 commits intomainfrom
fix/windows-hook-compat

Conversation

@Jamkris
Copy link
Copy Markdown
Owner

@Jamkris Jamkris commented Apr 23, 2026

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" <hook-name>

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.

Summary by CodeRabbit

  • Chores
    • Unified hook execution mechanism for improved reliability and maintainability.
    • Added cross-platform compatibility validation to prevent Windows PowerShell issues.
    • Expanded hook testing to verify correct execution paths and error handling behavior.

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" <hook-name>

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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

Warning

Rate limit exceeded

@Jamkris has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 51 minutes and 53 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 53 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 776f113b-31e4-44fd-900c-16bf2251b5ea

📥 Commits

Reviewing files that changed from the base of the PR and between 8c86836 and 6644ade.

📒 Files selected for processing (2)
  • scripts/hooks/run.js
  • tests/hooks/hooks.test.js

Walkthrough

Hook commands in hooks.json are refactored to route all actions through a unified run.js dispatcher instead of directly executing individual hook files, eliminating fallback path resolution and suppressing shell error handling mechanisms.

Changes

Cohort / File(s) Summary
Hook Configuration
hooks/hooks.json
Command strings updated for six hook actions (suggest-compact, pre-compact, session-start, check-console-log, session-end, evaluate-session) to invoke node run.js <action> instead of direct file execution with fallback paths and error suppression.
Hook Dispatcher
scripts/hooks/run.js
New Node.js launcher script that accepts a hook name argument, validates against safe pattern, resolves hook file relative to __dirname, spawns hook in separate process with inherited I/O, and conditionally logs errors only when GEMINI_HOOK_DEBUG=1 is set.
Hook Tests
tests/hooks/hooks.test.js
Added regression tests verifying hook commands avoid POSIX-only shell syntax (`

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

security

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main change: replacing POSIX-only shell syntax in hook commands with cross-platform Node.js launcher to fix Windows PowerShell compatibility (issue #42).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/windows-hook-compat

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@hooks/hooks.json`:
- Line 50: The hook command in hooks.json hardcodes the extension location
("node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/run.js\"
suggest-compact") and can fail for manual installs copied by scripts/install.sh;
update the hook to resolve install-mode-specific paths before invoking run.js
(try $HOME/.gemini/scripts/hooks/run.js first, then $HOME/.gemini/extensions/…,
or implement a small cross-platform node-resolver that picks the existing path)
and ensure the invoked run.js handles errors quietly and returns intentional
exit codes; reference and update the command strings in hooks.json, the run.js
launcher, and scripts/install.sh integration so the hook falls back safely and
does not leak errors to Gemini sessions.

In `@scripts/hooks/run.js`:
- Around line 37-41: The current spawnSync call can hang indefinitely; add a
timeout option (default 3000ms) so hung hooks are bounded: read an integer
timeout from process.env.GEMINI_HOOK_TIMEOUT (fallback 3000), pass it into the
spawnSync options for the call that uses process.execPath and hookPath, and keep
existing debug logging under GEMINI_HOOK_DEBUG for result.error or
result.signal; still call process.exit(0) after handling the result. Ensure you
reference the spawnSync invocation, the hookPath/process.execPath usage, and
preserve GEMINI_HOOK_DEBUG behavior while making the timeout configurable.

In `@tests/hooks/hooks.test.js`:
- Around line 390-395: The current assertion src.includes('__dirname') can be
satisfied by comments; change the test 'run.js launcher resolves hook files from
its own directory' to assert the actual implementation pattern or runtime
behavior: either (1) check the source for the explicit pattern
src.includes("path.join(__dirname") or src.includes("path.join(__dirname,") to
ensure path.join is used, or (2) execute the launcher (the launcher variable
pointing to 'scripts/hooks/run.js') with a known hook name via
child_process.spawnSync/execSync and assert the process reads argv[2] and
resolves a hook path relative to __dirname (non-zero exit or specific
stderr/stdout containing the expected resolved path). Update assertions
accordingly (replace src.includes('__dirname') and/or add the runtime
invocation) to reliably validate launcher behavior.
- Around line 370-375: Replace the naive substring checks in the test that use
bad = [' || ', '2>/dev/null', ' || true'] and hook.command.includes(pattern)
with regex-based guards so variants like "||true", "cmd|| true", and "2>
/dev/null" are caught; update the test to iterate over an array of RegExp
objects (e.g. patterns matching /\|\|\s*/, /\|\|\s*true\b/, /2>\s*\/dev\/null/i)
and assert that none of those regexes match hook.command using
RegExp.test(hook.command) (referencing the existing bad/pattern loop and
hook.command usage to locate where to change).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 81a65eb8-74a2-4aa1-b1a0-2be35aa7eb81

📥 Commits

Reviewing files that changed from the base of the PR and between 21071ff and 8c86836.

📒 Files selected for processing (3)
  • hooks/hooks.json
  • scripts/hooks/run.js
  • tests/hooks/hooks.test.js

Comment thread hooks/hooks.json
Comment thread scripts/hooks/run.js Outdated
Comment thread tests/hooks/hooks.test.js Outdated
Comment thread tests/hooks/hooks.test.js Outdated
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.
@Jamkris Jamkris merged commit 1fc8ce7 into main Apr 23, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant