fix(installer): preserve existing Claude hook settings#964
fix(installer): preserve existing Claude hook settings#964
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
affaan-m has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughInstaller now reads Changes
Sequence DiagramsequenceDiagram
participant Installer as Install Script
participant HooksFile as hooks/hooks.json
participant SettingsFile as ~/.claude/settings.json
participant FileSystem as File System
Installer->>HooksFile: Read hooks/hooks.json
activate HooksFile
HooksFile-->>Installer: Return hooks JSON (or error)
deactivate HooksFile
Installer->>Installer: Validate hooks shape
alt plan.adapter.target === "claude"
Installer->>SettingsFile: Read existing settings.json (if exists)
activate SettingsFile
SettingsFile-->>Installer: Return settings JSON (or missing/error)
deactivate SettingsFile
Installer->>Installer: Validate settings is object
Installer->>Installer: Merge incoming hooks into settings.hooks with per-event dedupe
Installer->>FileSystem: Ensure settings directory exists
activate FileSystem
FileSystem-->>Installer: Directory ready
deactivate FileSystem
Installer->>SettingsFile: Write merged settings.json
activate SettingsFile
SettingsFile-->>Installer: Write complete
deactivate SettingsFile
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue 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:51">
P2: Validate `settings.json` root type after parsing. Non-object JSON currently gets merged and rewritten instead of failing fast.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
affaan-m has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![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
🤖 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 77-84: The install currently mutates the target files before
validating settings which can leave a partial install; in applyInstallPlan, call
mergeHooksIntoSettings(plan) (or a new validateAndMergeSettings function) before
iterating plan.operations and performing fs.copyFileSync, so the Claude settings
are parsed/validated and any errors surface before touching files;
alternatively, stage writes by preparing temporary paths for each operation and
only perform the fs.mkdirSync/fs.copyFileSync loop after mergeHooksIntoSettings
succeeds, and ensure writeInstallState(plan.installStatePath, ...) still runs
only after successful copies.
- Around line 35-45: The parsed JSON root (hooksConfig) must be validated before
reading hooksConfig.hooks; update the parsing block to reject non-object roots
(null, array, or primitives) with a clear Error describing the file path and
expected shape, and then validate incomingHooks is an object (not array)
otherwise throw a descriptive installer error; apply the same pattern for the
analogous settings parsing (e.g., settingsConfig and incomingSettings) so both
code paths fail fast with explicit messages rather than silently returning or
throwing raw TypeErrors.
🪄 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: 97bc3558-8ee7-49b3-9cb1-5b4651ab3d5a
📒 Files selected for processing (2)
scripts/lib/install/apply.jstests/scripts/install-apply.test.js
| let hooksConfig; | ||
| try { | ||
| hooksConfig = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); | ||
| } catch (error) { | ||
| throw new Error(`Failed to parse hooks config at ${hooksJsonPath}: ${error.message}`); | ||
| } | ||
|
|
||
| const incomingHooks = hooksConfig.hooks; | ||
| if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Validate the parsed JSON root before touching .hooks.
JSON.parse() accepts null, arrays, and primitives. If hooks/hooks.json is null, Line 42 throws a raw TypeError; if settings.json is null, Line 57 does the same. Lines 43-45 also turn an invalid top-level hooks payload into a silent success. Reject non-object roots and invalid hooks shapes explicitly so this path always fails with a clear installer error.
🛡️ Proposed fix
try {
hooksConfig = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8'));
} catch (error) {
throw new Error(`Failed to parse hooks config at ${hooksJsonPath}: ${error.message}`);
}
+ if (!hooksConfig || typeof hooksConfig !== 'object' || Array.isArray(hooksConfig)) {
+ throw new Error(`Invalid hooks config at ${hooksJsonPath}: expected a JSON object`);
+ }
const incomingHooks = hooksConfig.hooks;
if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) {
- return;
+ throw new Error(`Invalid hooks config at ${hooksJsonPath}: expected a top-level "hooks" object`);
}
const settingsPath = path.join(plan.targetRoot, 'settings.json');
let settings = {};
if (fs.existsSync(settingsPath)) {
try {
settings = JSON.parse(fs.readFileSync(settingsPath, 'utf8'));
} catch (error) {
throw new Error(`Failed to parse existing settings at ${settingsPath}: ${error.message}`);
}
+ if (!settings || typeof settings !== 'object' || Array.isArray(settings)) {
+ throw new Error(`Invalid existing settings at ${settingsPath}: expected a JSON object`);
+ }
}As per coding guidelines, "Always validate all user input before processing at system boundaries", "Use schema-based validation where available", and "Fail fast with clear error messages when validation fails".
Also applies to: 49-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/install/apply.js` around lines 35 - 45, The parsed JSON root
(hooksConfig) must be validated before reading hooksConfig.hooks; update the
parsing block to reject non-object roots (null, array, or primitives) with a
clear Error describing the file path and expected shape, and then validate
incomingHooks is an object (not array) otherwise throw a descriptive installer
error; apply the same pattern for the analogous settings parsing (e.g.,
settingsConfig and incomingSettings) so both code paths fail fast with explicit
messages rather than silently returning or throwing raw TypeErrors.
|
Addressed the latest cubic finding in Verification:
@cubic-dev-ai please re-review the updated head. |
@affaan-m I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/lib/install/apply.js (1)
35-45:⚠️ Potential issue | 🟠 MajorMissing validation of
hooksConfigroot before accessing.hooks.The past review flagged this: if
hooks/hooks.jsoncontainsnull, an array, or a primitive,JSON.parsesucceeds buthooksConfig.hooks(line 42) throws a rawTypeErrorinstead of a clear installer error.The fix was applied for
settings.json(lines 52-54) but not here. Add the same guard after parsinghooksConfig.🛡️ Proposed fix
try { hooksConfig = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); } catch (error) { throw new Error(`Failed to parse hooks config at ${hooksJsonPath}: ${error.message}`); } + if (!hooksConfig || typeof hooksConfig !== 'object' || Array.isArray(hooksConfig)) { + throw new Error(`Invalid hooks config at ${hooksJsonPath}: expected a JSON object`); + } const incomingHooks = hooksConfig.hooks;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/install/apply.js` around lines 35 - 45, After parsing hooksConfig from hooksJsonPath, validate that hooksConfig is a plain object before accessing .hooks (same guard used for settings.json): if hooksConfig is falsy, not an object, or an array, throw a clear Error mentioning hooksJsonPath; then proceed to read const incomingHooks = hooksConfig.hooks and keep the existing checks for incomingHooks. Update the code around the hooksConfig parse (symbols: hooksConfig, hooksJsonPath, incomingHooks) to perform that guard and throw a descriptive installer error instead of allowing a raw TypeError.
🧹 Nitpick comments (1)
scripts/lib/install/apply.js (1)
8-23:JSON.stringifykey ordering may cause deduplication misses.
JSON.stringifyproduces different strings for semantically identical objects when property ordering differs (e.g.,{a:1, b:2}vs{b:2, a:1}). If hooks are created by different tools or manually edited, duplicates could slip through.Consider normalizing key order or using a deep-equals comparison if exact deduplication is required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/install/apply.js` around lines 8 - 23, mergeHookEntries currently deduplicates by JSON.stringify(entry) which can miss semantically identical objects with different property orders; change the deduplication key to a stable representation by normalizing object keys before stringifying (e.g., perform a deterministic, recursive key sort or use a stableStringify helper) or replace the Set lookup with a deep-equality check against mergedEntries; update references inside mergeHookEntries (entryKey, seenEntries, mergedEntries) to use the normalized key or deep-equal logic so objects with the same properties but different key orders are treated as duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/lib/install/apply.js`:
- Around line 35-45: After parsing hooksConfig from hooksJsonPath, validate that
hooksConfig is a plain object before accessing .hooks (same guard used for
settings.json): if hooksConfig is falsy, not an object, or an array, throw a
clear Error mentioning hooksJsonPath; then proceed to read const incomingHooks =
hooksConfig.hooks and keep the existing checks for incomingHooks. Update the
code around the hooksConfig parse (symbols: hooksConfig, hooksJsonPath,
incomingHooks) to perform that guard and throw a descriptive installer error
instead of allowing a raw TypeError.
---
Nitpick comments:
In `@scripts/lib/install/apply.js`:
- Around line 8-23: mergeHookEntries currently deduplicates by
JSON.stringify(entry) which can miss semantically identical objects with
different property orders; change the deduplication key to a stable
representation by normalizing object keys before stringifying (e.g., perform a
deterministic, recursive key sort or use a stableStringify helper) or replace
the Set lookup with a deep-equality check against mergedEntries; update
references inside mergeHookEntries (entryKey, seenEntries, mergedEntries) to use
the normalized key or deep-equal logic so objects with the same properties but
different key orders are treated as duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e7b0625-13e9-46cc-9466-6995153a9f60
📒 Files selected for processing (2)
scripts/lib/install/apply.jstests/scripts/install-apply.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/scripts/install-apply.test.js
There was a problem hiding this comment.
1 issue 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:42">
P2: Validate the parsed hooks config root and `hooks` shape before dereferencing; otherwise non-object JSON roots can throw a runtime `TypeError`, and invalid `hooks` payloads are silently ignored so installation appears successful without applying hooks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const incomingHooks = hooksConfig.hooks; | ||
| if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
P2: Validate the parsed hooks config root and hooks shape before dereferencing; otherwise non-object JSON roots can throw a runtime TypeError, and invalid hooks payloads are silently ignored so installation appears successful without applying hooks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/install/apply.js, line 42:
<comment>Validate the parsed hooks config root and `hooks` shape before dereferencing; otherwise non-object JSON roots can throw a runtime `TypeError`, and invalid `hooks` payloads are silently ignored so installation appears successful without applying hooks.</comment>
<file context>
@@ -1,15 +1,89 @@
+ throw new Error(`Failed to parse hooks config at ${hooksJsonPath}: ${error.message}`);
+ }
+
+ const incomingHooks = hooksConfig.hooks;
+ if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) {
+ return;
</file context>
| const incomingHooks = hooksConfig.hooks; | |
| if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) { | |
| return; | |
| } | |
| if (!hooksConfig || typeof hooksConfig !== 'object' || Array.isArray(hooksConfig)) { | |
| throw new Error(`Invalid hooks config at ${hooksJsonPath}: expected a JSON object`); | |
| } | |
| const incomingHooks = hooksConfig.hooks; | |
| if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) { | |
| throw new Error(`Invalid hooks config at ${hooksJsonPath}: expected a top-level "hooks" object`); | |
| } |
1 participant
Summary
hooks/hooks.jsoninto~/.claude/settings.jsonfor Claude installs without overwriting existing hook entries in the same event groupsettings.jsonwith a fresh fileWhy
#961identifies the real installer gap, but its current merge logic still overwrites existing arrays for shared hook events likePreToolUse. That makes the branch unsafe as a final merge target for users who already have custom Claude hooks.This PR supersedes
#961with a maintainer-owned branch that preserves existing event entries and adds explicit regression coverage for reinstall/idempotency and malformed settings handling.Verification
NODE_PATH=/Users/affoon/Documents/GitHub/ECC/everything-claude-code/node_modules node tests/scripts/install-apply.test.jsNODE_PATH=/Users/affoon/Documents/GitHub/ECC/everything-claude-code/node_modules node tests/lib/install-request.test.jsSupersedes
#961Summary by cubic
Preserves existing Claude hook settings during install by merging hooks into settings.json without overwriting user-defined entries. Reinstalls are idempotent and reject settings files whose root is not a JSON object.
Written for commit 971cc3c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests