-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(windows): Replace bash hook with Node.js for cross-platform support #356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fixes obra#354 The SessionStart hook was failing on Windows due to two issues: 1. Path mangling: When CLAUDE_PLUGIN_ROOT (containing Windows backslash paths) is passed through bash command execution, backslashes are interpreted as escape characters and stripped, resulting in invalid paths like "C:UsersSebastian.claude..." instead of "C:\Users\Sebastian\.claude..." 2. Shebang stdout suppression: Git Bash on Windows suppresses stdout when running scripts with shebang lines via `bash script.sh`, causing the hook to produce no output even when it executes successfully. Solution: Replace the bash script with a Node.js equivalent that: - Uses __dirname for reliable cross-platform path resolution - Works identically on Windows, macOS, and Linux - Produces proper JSON output for the Claude Code hook system The original session-start.sh is retained for reference and potential Unix-only use cases. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe SessionStart hook execution method is converted from a shell script to a Node.js script. The hooks configuration is updated to invoke the JavaScript file instead of the bash script, providing cross-platform execution without shebang-related issues. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hooks/hooks.json`:
- Around line 8-10: The command string in the hooks.json entry uses double
quotes around ${CLAUDE_PLUGIN_ROOT} which can allow word-splitting for paths
with spaces; update the "command" value so the expanded ${CLAUDE_PLUGIN_ROOT} is
wrapped in single quotes (preserving the existing escaped inner path to
hooks/session-start.js) so the shell treats the full path as a single
literal—look for the "command" key that references
"${CLAUDE_PLUGIN_ROOT}/hooks/session-start.js" and replace the surrounding
double quotes for the variable portion with single quotes.
🧹 Nitpick comments (1)
hooks/session-start.js (1)
20-22: Useos.homedir()to avoid empty/relative legacy path resolution.If HOME/USERPROFILE is unset, the current code resolves to a relative
.config/...path, which can cause false warnings or filesystem probes in the working directory.os.homedir()is the reliable cross‑platform source.♻️ Suggested tweak
-const legacySkillsDir = path.join(process.env.HOME || process.env.USERPROFILE || '', '.config', 'superpowers', 'skills'); -if (fs.existsSync(legacySkillsDir)) { +const homeDir = require('os').homedir(); +const legacySkillsDir = homeDir ? path.join(homeDir, '.config', 'superpowers', 'skills') : ''; +if (legacySkillsDir && fs.existsSync(legacySkillsDir)) { warningMessage = '\n\n<important-reminder>IN YOUR FIRST REPLY AFTER SEEING THIS MESSAGE YOU MUST TELL THE USER:⚠️ **WARNING:** Superpowers now uses Claude Code\'s skills system. Custom skills in ~/.config/superpowers/skills will not be read. Move custom skills to ~/.claude/skills instead. To make this message go away, remove ~/.config/superpowers/skills</important-reminder>'; }
| "type": "command", | ||
| "command": "${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh" | ||
| "command": "node \"${CLAUDE_PLUGIN_ROOT}/hooks/session-start.js\"" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# Check the repository structure and hooks.json file
git ls-files hooks/ 2>/dev/null | head -20Repository: obra/superpowers
Length of output: 141
🏁 Script executed:
# Read the hooks.json file to see the current state
if [ -f "hooks/hooks.json" ]; then
cat -n hooks/hooks.json
fiRepository: obra/superpowers
Length of output: 443
🏁 Script executed:
# Check if this is a superpowers plugin repo
git ls-files | grep -E "(package\.json|README|superpowers)" | head -20Repository: obra/superpowers
Length of output: 266
Use single quotes around ${CLAUDE_PLUGIN_ROOT} to prevent word splitting in paths with spaces.
Claude Code expands variables before passing the command to the shell, so single quotes preserve the expanded path as a literal string and prevent breaking on spaces. Double-quoted escaping does not provide the same protection.
✅ Suggested change
- "command": "node \"${CLAUDE_PLUGIN_ROOT}/hooks/session-start.js\""
+ "command": "node '${CLAUDE_PLUGIN_ROOT}/hooks/session-start.js'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "type": "command", | |
| "command": "${CLAUDE_PLUGIN_ROOT}/hooks/session-start.sh" | |
| "command": "node \"${CLAUDE_PLUGIN_ROOT}/hooks/session-start.js\"" | |
| } | |
| "type": "command", | |
| "command": "node '${CLAUDE_PLUGIN_ROOT}/hooks/session-start.js'" | |
| } |
🤖 Prompt for AI Agents
In `@hooks/hooks.json` around lines 8 - 10, The command string in the hooks.json
entry uses double quotes around ${CLAUDE_PLUGIN_ROOT} which can allow
word-splitting for paths with spaces; update the "command" value so the expanded
${CLAUDE_PLUGIN_ROOT} is wrapped in single quotes (preserving the existing
escaped inner path to hooks/session-start.js) so the shell treats the full path
as a single literal—look for the "command" key that references
"${CLAUDE_PLUGIN_ROOT}/hooks/session-start.js" and replace the surrounding
double quotes for the variable portion with single quotes.
Summary
Fixes #354
The SessionStart hook was failing on Windows due to two distinct issues:
Issue 1: Path Mangling
When
CLAUDE_PLUGIN_ROOT(containing Windows backslash paths likeC:\Users\...) is passed through bash command execution, backslashes are interpreted as escape characters and stripped:Issue 2: Shebang stdout suppression
Git Bash on Windows suppresses stdout when running scripts with shebang lines via
bash script.sh:Solution
Replace the bash script with a Node.js equivalent that:
__dirnamefor reliable cross-platform path resolutionThe original
session-start.shis retained for reference and potential Unix-only use cases.Test Plan
Related Issues
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.