Skip to content

fix: preserve existing hooks on install#145

Open
zerone0x wants to merge 1 commit intotirth8205:mainfrom
zerone0x:fix/hooks-backup-114
Open

fix: preserve existing hooks on install#145
zerone0x wants to merge 1 commit intotirth8205:mainfrom
zerone0x:fix/hooks-backup-114

Conversation

@zerone0x
Copy link
Copy Markdown
Contributor

@zerone0x zerone0x commented Apr 8, 2026

Summary

  • back up existing .claude/settings.json before writing hooks
  • merge new hooks with existing entries instead of overwriting them
  • add tests for preserving hooks and backup creation

Fixes #114.

Testing

  • uv run pytest tests/test_skills.py -q

Co-Authored-By: Claude <noreply@anthropic.com>
@tirth8205
Copy link
Copy Markdown
Owner

CI passed, but this now has merge conflicts with skills.py after PR #152 merged. Could you rebase on latest main? Once rebased, we'll merge immediately.

lngyeen added a commit to lngyeen/code-review-graph that referenced this pull request Apr 10, 2026
Two fixes for Claude Code hooks integration:

1. Hook schema: use proper Claude Code format with nested hooks array
   and remove unsupported PreCommit event. Narrow PostToolUse matcher
   from Edit|Write|Bash to Edit|Write since Bash commands do not
   directly modify source files. (cherry-picked from PR tirth8205#180)

2. Merge logic: install_hooks now merges new entries into existing
   hooks instead of overwriting them. Creates a backup of
   settings.json before modification. (based on PR tirth8205#145)

Closes tirth8205#97, tirth8205#114, tirth8205#172.
lngyeen added a commit to lngyeen/code-review-graph that referenced this pull request Apr 10, 2026
Two fixes for Claude Code hooks integration:

1. Hook schema: use proper Claude Code format with nested hooks array
   and remove unsupported PreCommit event. Narrow PostToolUse matcher
   from Edit|Write|Bash to Edit|Write since Bash commands do not
   directly modify source files. (cherry-picked from PR tirth8205#180)

2. Merge logic: install_hooks now merges new entries into existing
   hooks instead of overwriting them. Creates a backup of
   settings.json before modification. (based on PR tirth8205#145)

Closes tirth8205#97, tirth8205#114, tirth8205#172.
@tirth8205
Copy link
Copy Markdown
Owner

Thanks for tackling issue #114! The merge logic and backup approach are the right solution. However there are two bugs that need fixing before this can merge:

1. Missing encoding="utf-8" on write (regression)

The original install_hooks wrote with encoding="utf-8":

settings_path.write_text(json.dumps(existing, indent=2) + "\n", encoding="utf-8")

The PR drops the encoding parameter:

settings_path.write_text(json.dumps(existing, indent=2) + "\n")

On Windows with non-UTF-8 system locale this will break. Please restore encoding="utf-8".

2. Hook format generated is invalid

The PR still calls the original generate_hooks_config() which emits:

  • A PreCommit hook — this is not a valid Claude Code hook event
  • Flat {"matcher": ..., "command": ..., "timeout": ...} structure instead of the correct nested {"matcher": ..., "hooks": [{"type": "command", "command": ..., "timeout": ...}]} structure

These bugs were also present before this PR, but since you're changing the merge logic it would be good to fix the format here too. PR #192 has the correct format if you want to pull from it.

The backup creation and merge loop logic itself is correct — just these two things to fix.

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.

code-review-graph install overwrites existing hooks for Claude

2 participants