fix(hooks): merge hooks config into settings.json for claude target#961
fix(hooks): merge hooks config into settings.json for claude target#961shixi-li wants to merge 2 commits intoaffaan-m:mainfrom
Conversation
The installer copies hooks/hooks.json to ~/.claude/hooks/hooks.json, but Claude Code only reads hooks from ~/.claude/settings.json. This causes all 28 hooks to be silently inactive after installation. Add a post-install merge step in applyInstallPlan() that writes the hooks field from hooks.json into settings.json, preserving any existing user settings. The merge only runs for the claude target.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates Changes
Sequence Diagram(s)sequenceDiagram
actor CLI
participant Installer as Installer (applyInstallPlan)
participant FS as FileSystem
participant Settings as settings.json
participant Hooks as hooks/hooks.json
CLI->>Installer: run install plan
Installer->>FS: copy plan.operations files (ensure parent dirs via path.dirname)
Installer->>Hooks: read hooks/hooks.json (if target === "claude")
alt hooks file exists and parses
Hooks-->>Installer: hooksConfig.hooks
Installer->>Settings: read existing settings.json (if exists)
alt settings parsed
Settings-->>Installer: current settings object
else parse error or missing
Settings-->>Installer: {} (fallback)
end
Installer->>Settings: write merged settings (ensure parent dir)
else missing hooks file
Hooks-->>Installer: no-op (return)
end
Installer->>CLI: finish
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a real bug where all 28 ECC hooks were silently inactive after installation for the Confidence Score: 4/5Safe to merge for most users, but users with pre-existing ECC-defined hook event types in settings.json will have those entries silently replaced on every reinstall. The bug fix is correct and tests pass. The two new findings are P2. However, the previously-flagged P1 shallow-merge issue — user-defined hooks for event types shared with ECC (e.g. PreToolUse) are overwritten on reinstall with no warning — remains unaddressed, keeping the score at 4. scripts/lib/install/apply.js — the merge strategy on line 46 and the Array guard on line 31. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as install-apply.js
participant Apply as applyInstallPlan()
participant FS as File System
participant Merge as mergeHooksIntoSettings()
CLI->>Apply: applyInstallPlan(plan)
loop For each operation
Apply->>FS: mkdirSync(dest dir)
Apply->>FS: copyFileSync(src → dest)
end
Apply->>Merge: mergeHooksIntoSettings(plan)
Merge->>Merge: check plan.adapter.target === 'claude'
Merge->>FS: existsSync(hooks/hooks.json)
FS-->>Merge: true
Merge->>FS: readFileSync(hooks.json)
FS-->>Merge: hooksConfig
Merge->>FS: existsSync(settings.json)
alt settings.json exists
Merge->>FS: readFileSync(settings.json)
FS-->>Merge: existing settings
end
Merge->>Merge: settings.hooks = { ...settings.hooks, ...hooksConfig.hooks }
Merge->>FS: writeFileSync(settings.json)
Apply->>FS: writeInstallState()
Apply-->>CLI: { ...plan, applied: true }
Reviews (2): Last reviewed commit: "fix: merge hooks per event type and warn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/scripts/install-apply.test.js (1)
354-374: Strengthen preservation test to include pre-existingsettings.hooks.Right now this only proves non-hook fields survive. It won’t catch regressions where existing user hook groups are overwritten during merge.
✅ Suggested test enhancement
fs.writeFileSync( path.join(claudeRoot, 'settings.json'), - JSON.stringify({ effortLevel: 'high', env: { MY_VAR: '1' } }, null, 2) + JSON.stringify({ + effortLevel: 'high', + env: { MY_VAR: '1' }, + hooks: { + UserPromptSubmit: [{ matcher: '*', hooks: [{ type: 'command', command: 'echo custom' }] }], + }, + }, null, 2) ); @@ assert.strictEqual(settings.effortLevel, 'high', 'existing effortLevel should be preserved'); assert.deepStrictEqual(settings.env, { MY_VAR: '1' }, 'existing env should be preserved'); assert.ok(settings.hooks, 'hooks should be merged in'); assert.ok(settings.hooks.PreToolUse, 'PreToolUse hooks should exist'); + assert.ok(settings.hooks.UserPromptSubmit, 'existing hooks should be preserved');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/install-apply.test.js` around lines 354 - 374, Update the test "preserves existing settings.json fields when merging hooks" to seed settings.json with an existing hooks object (e.g., include a hooks.PreToolUse entry) before running run([...]) so the test verifies merging rather than only non-hook fields; specifically, in the setup that writes to path.join(claudeRoot, 'settings.json') (inside the test function using homeDir/claudeRoot variables) include a hooks property with at least one group and entry, then after run() assert that those pre-existing hook groups/entries still exist and that new hooks were merged (e.g., check settings.hooks.PreToolUse contains both the original entry and any new entries and that no entire hook group was overwritten).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lib/install/apply.js`:
- Around line 24-28: The code currently swallows JSON.parse failures when
reading hooksJsonPath (assigning hooksConfig), which can hide invalid installs;
update the catch blocks around JSON.parse(fs.readFileSync(hooksJsonPath,
'utf8')) so they do not silently return—either rethrow a new Error with
contextual information (including hooksJsonPath and the original error.message)
or call the module's logger to log a clear failure before exiting; ensure the
same change is applied to the other try/catch that reads/parses hooksJsonPath so
invalid JSON fails fast with a descriptive message rather than silently
continuing.
- Around line 44-47: Current code overwrites settings.hooks with
hooksConfig.hooks which can drop existing user hook groups; instead merge
immutably by creating a new hooks object that combines existing settings.hooks
and hooksConfig.hooks (preserving existing entries and only adding/updating
incoming ones) and assign that to settings.hooks before serializing; refer to
settings.hooks and hooksConfig.hooks in your change and ensure you do not mutate
the original settings object (create a new object/clone for hooks), then write
the merged settings to settingsPath with fs.writeFileSync as before.
---
Nitpick comments:
In `@tests/scripts/install-apply.test.js`:
- Around line 354-374: Update the test "preserves existing settings.json fields
when merging hooks" to seed settings.json with an existing hooks object (e.g.,
include a hooks.PreToolUse entry) before running run([...]) so the test verifies
merging rather than only non-hook fields; specifically, in the setup that writes
to path.join(claudeRoot, 'settings.json') (inside the test function using
homeDir/claudeRoot variables) include a hooks property with at least one group
and entry, then after run() assert that those pre-existing hook groups/entries
still exist and that new hooks were merged (e.g., check
settings.hooks.PreToolUse contains both the original entry and any new entries
and that no entire hook group was overwritten).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d13c959c-bc1f-4299-b43c-a6ee24f745d5
📒 Files selected for processing (2)
scripts/lib/install/apply.jstests/scripts/install-apply.test.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/lib/install/apply.js">
<violation number="1" location="scripts/lib/install/apply.js:40">
P1: Malformed `settings.json` is silently replaced with `{}` + hooks, risking user setting loss; parse failures are swallowed instead of failing install.</violation>
<violation number="2" location="scripts/lib/install/apply.js:44">
P1: Directly assigning `settings.hooks` replaces the entire existing hooks configuration and can delete user-defined hooks on reinstall. Merge with the current `settings.hooks` object instead of overwriting it wholesale.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Address review feedback from CodeRabbit, Greptile, and Cubic:
- Use spread merge ({ ...existing, ...incoming }) instead of full
replacement so user-defined hook event types (e.g. UserPromptSubmit)
survive reinstallation. ECC-managed event types still update.
- Log warnings to stderr when hooks.json or settings.json contain
invalid JSON, instead of silently swallowing parse errors.
- Strengthen test to seed settings.json with a pre-existing hooks
entry and verify it is preserved after install.
|
Superseded by #964. |
2 participants
Summary
The installer copies
hooks/hooks.jsonto~/.claude/hooks/hooks.json, but Claude Code only reads hooks from~/.claude/settings.json. This means all 28 hooks (PreToolUse, PostToolUse, Stop, SessionStart, etc.) are silently inactive after installation for theclaudetarget.This fix adds a post-install merge step in
applyInstallPlan()that writes thehooksfield fromhooks.jsonintosettings.json, preserving any existing user settings. The merge only runs for theclaudetarget — Cursor and other targets are unaffected.Root cause
hooks-runtimemodule paths (hooks,scripts/hooks,scripts/lib) are copied to~/.claude/as-isclaude-home.js) has no customplanOperations()to handle hooks speciallyhooks.json), Claude Code requires hooks insettings.jsonChanges
scripts/lib/install/apply.js: AddedmergeHooksIntoSettings(plan)— after file copies complete, reads~/.claude/hooks/hooks.json, extracts thehooksobject, and merges it into~/.claude/settings.json(preserving existing settings)tests/scripts/install-apply.test.js: Added 3 test cases — hooks merge for claude target, preserving existing settings, and skipping for non-claude targetsType
Testing
--profile fullinstall on Windowssettings.jsonafter installChecklist
Summary by cubic
Fixes inactive hooks for the
claudetarget by merging hooks from~/.claude/hooks/hooks.jsoninto~/.claude/settings.jsonafter install. Preserves user-defined hook event types on reinstall; other targets are unaffected.applyInstallPlan()to writehooksintosettings.json; preserves existing fields and user-defined hook types, createssettings.jsonif missing, and warns to stderr on JSON parse errors.claudeskip.Written for commit 46329ae. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests