fix(windows): SessionStart hook fails with Git Bash paths#507
fix(windows): SessionStart hook fails with Git Bash paths#507shlokmestry wants to merge 1 commit intoobra:mainfrom
Conversation
📝 WalkthroughWalkthroughThe SessionStart hook command in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hooks/hooks.json (1)
9-9: Consider dropping the-l(login) flag frombash -lc
-lcauses bash to source/etc/profileand~/.bash_profileon every hook invocation. On macOS/Linux this is unnecessary overhead and can trigger side effects from user login scripts (conda/pyenv/nvm initialisers,cdcalls, early exits, etc.). On Windows/Git Bash,cygpathlives in/usr/binand is typically present in PATH even without a login shell, so-llikely isn't required there either.If
cygpathis consistently found without a login shell (as it is in standard Git Bash installations), change-lcto-c:♻️ Proposed refactor
- "command": "bash -lc 'ROOT=\"${CLAUDE_PLUGIN_ROOT}\"; if command -v cygpath >/dev/null 2>&1; then ROOT=\"$(cygpath -u \"$ROOT\")\"; fi; \"$ROOT/hooks/session-start.sh\"'", + "command": "bash -c 'ROOT=\"${CLAUDE_PLUGIN_ROOT}\"; if command -v cygpath >/dev/null 2>&1; then ROOT=\"$(cygpath -u \"$ROOT\")\"; fi; \"$ROOT/hooks/session-start.sh\"'",If there is a confirmed reason that
cygpathrequires a login shell on the target Windows environment, add a comment in the PR description documenting that requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/hooks.json` at line 9, The hook command currently invokes bash with the login flag (`bash -lc '...') which forces sourcing of login profiles and can cause side effects; update the "command" entry in hooks.json to use `bash -c` instead of `bash -lc` so the hook runs a non-login shell (keep the existing cygpath handling and $CLAUDE_PLUGIN_ROOT expansion and the call to "hooks/session-start.sh"); if there's a confirmed environment-specific need for a login shell (e.g., cygpath only available when sourcing profiles), add a brief comment in the PR explaining that constraint instead of keeping `-l`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hooks/hooks.json`:
- Line 9: The hook command currently invokes bash with the login flag (`bash -lc
'...') which forces sourcing of login profiles and can cause side effects;
update the "command" entry in hooks.json to use `bash -c` instead of `bash -lc`
so the hook runs a non-login shell (keep the existing cygpath handling and
$CLAUDE_PLUGIN_ROOT expansion and the call to "hooks/session-start.sh"); if
there's a confirmed environment-specific need for a login shell (e.g., cygpath
only available when sourcing profiles), add a brief comment in the PR explaining
that constraint instead of keeping `-l`.
1 participant
Motivation and Context
On Windows, the
SessionStarthook fails because${CLAUDE_PLUGIN_ROOT}resolves to a Windows-style path (e.g.C:\Users\...), which Git Bash cannot execute directly as a script path.This causes
session-start.shto fail on every Claude startup.This change runs the hook through
bash -lcand converts the plugin root to a Unix-style path usingcygpath -uwhen available.How Has This Been Tested?
cygpathconversion only executes when available (Windows Git Bash)cygpath, behavior remains unchangedBreaking Changes
None.
Types of Changes
Additional Context
This provides a minimal cross-platform fix without introducing OS-specific branching or additional wrapper scripts.
Fixes #504
Summary by CodeRabbit