fix(plugin): quote HOOK_COMMAND path to handle spaces in home directory#1056
Conversation
…ry (#993) Use $HOME with double quotes instead of unquoted ~ to prevent breakage when the home directory path contains spaces.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
JeremyDev87
left a comment
JeremyDev87
left a comment
There was a problem hiding this comment.
Code Review — PR #1056
Critical: Backward Compatibility Bug in _is_hook_in_settings (line 274)
_is_hook_in_settings() at line 274 uses exact string match:
if hook.get("command") == HOOK_COMMAND:Existing installations have python3 ~/.claude/hooks/codingbuddy-mode-detect.py stored in settings.json. After this PR, HOOK_COMMAND becomes python3 "$HOME/.claude/hooks/codingbuddy-mode-detect.py".
Impact: The comparison will FAIL for existing users → duplicate hook entry added to settings.json → hook runs twice per prompt.
Suggested fix: Match by HOOK_FILENAME substring instead of exact command match:
def _is_hook_in_settings(settings: dict) -> bool:
user_prompt_hooks = settings.get("hooks", {}).get("UserPromptSubmit", [])
for hook_group in user_prompt_hooks:
for hook in hook_group.get("hooks", []):
if HOOK_FILENAME in hook.get("command", ""):
return True
return FalseThis handles both old (~) and new ($HOME) formats.
Minor: permission_hint messages still use ~
Lines 42, 51, 60, 69, 78 display ~/.claude/hooks/... in hint messages. While these are display-only, consider updating them to $HOME for consistency, or leave as-is since ~ is more readable for users.
Good
- Correct use of
$HOMEinstead of~(proper quoting in all shell contexts) - Double-quote wrapping ensures paths with spaces work
- Minimal, focused change
… compat Existing installations store the old ~ format in settings.json. Exact string comparison against the new $HOME format would fail, causing duplicate hook registration. Match by HOOK_FILENAME substring instead.
Review ResponseCritical: Backward Compatibility Bug — ResolvedFixed in commit 446db60. Changed # Before
if hook.get("command") == HOOK_COMMAND:
# After
if HOOK_FILENAME in hook.get("command", ""):This handles both old ( Minor:
|
JeremyDev87
left a comment
JeremyDev87
left a comment
There was a problem hiding this comment.
Re-Review: APPROVED ✅
Review feedback fully addressed:
- Critical fix applied:
_is_hook_in_settings()now usesHOOK_FILENAME in hook.get("command", "")instead of exact string match — backward compatibility preserved for existing installations. HOOK_COMMANDproperly uses$HOMEwith double-quote wrapping.
No critical or high issues remaining. Ready to merge.
1 participant
Summary
HOOK_COMMANDpath using$HOMEwith double quotes instead of unquoted~Fixes #993
Test plan
yarn test:hooks— 389 tests passed