Skip to content

fix(installer): preserve existing Claude hook settings #964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
affaan-m merged 3 commits into from
Mar 28, 2026
+261 −1
Merged

fix(installer): preserve existing Claude hook settings #964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f37a5d
fix(installer): preserve existing claude hook settings
affaan-m Mar 27, 2026
d7e6bb2
fix(installer): reject invalid claude settings roots
affaan-m Mar 27, 2026
47aa415
fix(installer): validate hooks and settings before install
affaan-m Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file modified scripts/ecc.js
100644 → 100755
View file
Open in desktop
Empty file.
Empty file modified scripts/install-apply.js
100644 → 100755
View file
Open in desktop
Empty file.
96 changes: 95 additions & 1 deletion scripts/lib/install/apply.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,109 @@
'use strict';

const fs = require('fs');
const path = require('path');

const { writeInstallState } = require('../install-state');

function readJsonObject(filePath, label) {
let parsed;
try {
parsed = JSON.parse(fs.readFileSync(filePath, 'utf8'));
} catch (error) {
throw new Error(`Failed to parse ${label} at ${filePath}: ${error.message}`);
}

if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
throw new Error(`Invalid ${label} at ${filePath}: expected a JSON object`);
}

return parsed;
}

function mergeHookEntries(existingEntries, incomingEntries) {
const mergedEntries = [];
const seenEntries = new Set();

for (const entry of [...existingEntries, ...incomingEntries]) {
const entryKey = JSON.stringify(entry);
if (seenEntries.has(entryKey)) {
continue;
}

seenEntries.add(entryKey);
mergedEntries.push(entry);
}

return mergedEntries;
}

function findHooksSourcePath(plan, hooksDestinationPath) {
const operation = plan.operations.find(item => item.destinationPath === hooksDestinationPath);
return operation ? operation.sourcePath : null;
}

function buildMergedSettings(plan) {
if (!plan.adapter || plan.adapter.target !== 'claude') {
return null;
}

const hooksDestinationPath = path.join(plan.targetRoot, 'hooks', 'hooks.json');
const hooksSourcePath = findHooksSourcePath(plan, hooksDestinationPath) || hooksDestinationPath;
if (!fs.existsSync(hooksSourcePath)) {
return null;
}
Comment on lines +50 to +54
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent null return when plan-listed hooks source does not exist on disk

When findHooksSourcePath returns a non-null path (because an operation for hooks.json exists in the plan), but that source file is absent on disk, fs.existsSync returns false and buildMergedSettings silently returns null. As a result:

  1. No hooks are merged into settings.json — the operation the plan explicitly requested is skipped with no diagnostic.
  2. The subsequent copyFileSync call in the operations loop will throw a generic "no such file" error instead of a clear, actionable message.

Consider raising an explicit error when the source path comes from the plan (meaning it is expected to exist):

const hooksSourcePath = findHooksSourcePath(plan, hooksDestinationPath) || hooksDestinationPath;
const hooksSourceIsFromPlan = !!findHooksSourcePath(plan, hooksDestinationPath);

if (!fs.existsSync(hooksSourcePath)) {
  if (hooksSourceIsFromPlan) {
    throw new Error(`Hooks source not found at ${hooksSourcePath}`);
  }
  return null;
}


const hooksConfig = readJsonObject(hooksSourcePath, 'hooks config');
const incomingHooks = hooksConfig.hooks;
if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) {
throw new Error(`Invalid hooks config at ${hooksSourcePath}: expected "hooks" to be a JSON object`);
}

const settingsPath = path.join(plan.targetRoot, 'settings.json');
let settings = {};
if (fs.existsSync(settingsPath)) {
settings = readJsonObject(settingsPath, 'existing settings');
}

const existingHooks = settings.hooks && typeof settings.hooks === 'object' && !Array.isArray(settings.hooks)
? settings.hooks
: {};
const mergedHooks = { ...existingHooks };

for (const [eventName, incomingEntries] of Object.entries(incomingHooks)) {
const currentEntries = Array.isArray(existingHooks[eventName]) ? existingHooks[eventName] : [];
const nextEntries = Array.isArray(incomingEntries) ? incomingEntries : [];
mergedHooks[eventName] = mergeHookEntries(currentEntries, nextEntries);
}

const mergedSettings = {
...settings,
hooks: mergedHooks,
};

return {
settingsPath,
mergedSettings,
};
}

function applyInstallPlan(plan) {
const mergedSettingsPlan = buildMergedSettings(plan);

for (const operation of plan.operations) {
fs.mkdirSync(require('path').dirname(operation.destinationPath), { recursive: true });
fs.mkdirSync(path.dirname(operation.destinationPath), { recursive: true });
fs.copyFileSync(operation.sourcePath, operation.destinationPath);
}

if (mergedSettingsPlan) {
fs.mkdirSync(path.dirname(mergedSettingsPlan.settingsPath), { recursive: true });
fs.writeFileSync(
mergedSettingsPlan.settingsPath,
JSON.stringify(mergedSettingsPlan.mergedSettings, null, 2) + '\n',
'utf8'
);
}

writeInstallState(plan.installStatePath, plan.statePreview);

return {
Expand Down
166 changes: 166 additions & 0 deletions tests/scripts/install-apply.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ function readJson(filePath) {
return JSON.parse(fs.readFileSync(filePath, 'utf8'));
}

const REPO_ROOT = path.join(__dirname, '..', '..');

function run(args = [], options = {}) {
const env = {
...process.env,
Expand Down Expand Up @@ -326,6 +328,170 @@ function runTests() {
assert.ok(result.stderr.includes('Unknown install module: ghost-module'));
})) passed++; else failed++;

if (test('merges hooks into settings.json for claude target install', () => {
const homeDir = createTempDir('install-apply-home-');
const projectDir = createTempDir('install-apply-project-');

try {
const result = run(['--profile', 'core'], { cwd: projectDir, homeDir });
assert.strictEqual(result.code, 0, result.stderr);

const claudeRoot = path.join(homeDir, '.claude');
assert.ok(fs.existsSync(path.join(claudeRoot, 'hooks', 'hooks.json')), 'hooks.json should be copied');

const settingsPath = path.join(claudeRoot, 'settings.json');
assert.ok(fs.existsSync(settingsPath), 'settings.json should exist after install');

const settings = readJson(settingsPath);
assert.ok(settings.hooks, 'settings.json should contain hooks key');
assert.ok(settings.hooks.PreToolUse, 'hooks should include PreToolUse');
assert.ok(Array.isArray(settings.hooks.PreToolUse), 'PreToolUse should be an array');
assert.ok(settings.hooks.PreToolUse.length > 0, 'PreToolUse should have entries');
} finally {
cleanup(homeDir);
cleanup(projectDir);
}
})) passed++; else failed++;

if (test('preserves existing settings fields and hook entries when merging hooks', () => {
const homeDir = createTempDir('install-apply-home-');
const projectDir = createTempDir('install-apply-project-');

try {
const claudeRoot = path.join(homeDir, '.claude');
fs.mkdirSync(claudeRoot, { recursive: true });
fs.writeFileSync(
path.join(claudeRoot, 'settings.json'),
JSON.stringify({
effortLevel: 'high',
env: { MY_VAR: '1' },
hooks: {
PreToolUse: [{ matcher: 'Write', hooks: [{ type: 'command', command: 'echo custom-pretool' }] }],
UserPromptSubmit: [{ matcher: '*', hooks: [{ type: 'command', command: 'echo custom-submit' }] }],
},
}, null, 2)
);

const result = run(['--profile', 'core'], { cwd: projectDir, homeDir });
assert.strictEqual(result.code, 0, result.stderr);

const settings = readJson(path.join(claudeRoot, 'settings.json'));
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.PreToolUse.some(entry => JSON.stringify(entry).includes('echo custom-pretool')),
'existing PreToolUse entries should be preserved'
);
assert.ok(settings.hooks.PreToolUse.length > 1, 'ECC PreToolUse hooks should be appended');
assert.deepStrictEqual(
settings.hooks.UserPromptSubmit,
[{ matcher: '*', hooks: [{ type: 'command', command: 'echo custom-submit' }] }],
'user-defined hook event types should be preserved'
);
} finally {
cleanup(homeDir);
cleanup(projectDir);
}
})) passed++; else failed++;

if (test('reinstall does not duplicate managed hook entries', () => {
const homeDir = createTempDir('install-apply-home-');
const projectDir = createTempDir('install-apply-project-');

try {
const firstInstall = run(['--profile', 'core'], { cwd: projectDir, homeDir });
assert.strictEqual(firstInstall.code, 0, firstInstall.stderr);

const settingsPath = path.join(homeDir, '.claude', 'settings.json');
const afterFirstInstall = readJson(settingsPath);
const preToolUseLength = afterFirstInstall.hooks.PreToolUse.length;

const secondInstall = run(['--profile', 'core'], { cwd: projectDir, homeDir });
assert.strictEqual(secondInstall.code, 0, secondInstall.stderr);

const afterSecondInstall = readJson(settingsPath);
assert.strictEqual(
afterSecondInstall.hooks.PreToolUse.length,
preToolUseLength,
'managed hook entries should not duplicate on reinstall'
);
} finally {
cleanup(homeDir);
cleanup(projectDir);
}
})) passed++; else failed++;

if (test('fails when existing settings.json is malformed', () => {
const homeDir = createTempDir('install-apply-home-');
const projectDir = createTempDir('install-apply-project-');

try {
const claudeRoot = path.join(homeDir, '.claude');
fs.mkdirSync(claudeRoot, { recursive: true });
const settingsPath = path.join(claudeRoot, 'settings.json');
fs.writeFileSync(settingsPath, '{ invalid json\n');

const result = run(['--profile', 'core'], { cwd: projectDir, homeDir });
assert.strictEqual(result.code, 1);
assert.ok(result.stderr.includes('Failed to parse existing settings at'));
assert.strictEqual(fs.readFileSync(settingsPath, 'utf8'), '{ invalid json\n');
assert.ok(!fs.existsSync(path.join(claudeRoot, 'hooks', 'hooks.json')), 'hooks.json should not be copied on validation failure');
assert.ok(!fs.existsSync(path.join(claudeRoot, 'ecc', 'install-state.json')), 'install state should not be written on validation failure');
} finally {
cleanup(homeDir);
cleanup(projectDir);
}
})) passed++; else failed++;

if (test('fails when existing settings.json root is not an object', () => {
const homeDir = createTempDir('install-apply-home-');
const projectDir = createTempDir('install-apply-project-');

try {
const claudeRoot = path.join(homeDir, '.claude');
fs.mkdirSync(claudeRoot, { recursive: true });
const settingsPath = path.join(claudeRoot, 'settings.json');
fs.writeFileSync(settingsPath, '[]\n');

const result = run(['--profile', 'core'], { cwd: projectDir, homeDir });
assert.strictEqual(result.code, 1);
assert.ok(result.stderr.includes('Invalid existing settings at'));
assert.ok(result.stderr.includes('expected a JSON object'));
assert.strictEqual(fs.readFileSync(settingsPath, 'utf8'), '[]\n');
assert.ok(!fs.existsSync(path.join(claudeRoot, 'hooks', 'hooks.json')), 'hooks.json should not be copied on validation failure');
assert.ok(!fs.existsSync(path.join(claudeRoot, 'ecc', 'install-state.json')), 'install state should not be written on validation failure');
} finally {
cleanup(homeDir);
cleanup(projectDir);
}
})) passed++; else failed++;

if (test('fails when source hooks.json root is not an object before copying files', () => {
const homeDir = createTempDir('install-apply-home-');
const projectDir = createTempDir('install-apply-project-');
const sourceHooksPath = path.join(REPO_ROOT, 'hooks', 'hooks.json');
const originalHooks = fs.readFileSync(sourceHooksPath, 'utf8');

try {
fs.writeFileSync(sourceHooksPath, '[]\n');

const result = run(['--profile', 'core'], { cwd: projectDir, homeDir });
assert.strictEqual(result.code, 1);
assert.ok(result.stderr.includes('Invalid hooks config at'));
assert.ok(result.stderr.includes('expected a JSON object'));

const claudeRoot = path.join(homeDir, '.claude');
assert.ok(!fs.existsSync(path.join(claudeRoot, 'hooks', 'hooks.json')), 'hooks.json should not be copied when source hooks are invalid');
assert.ok(!fs.existsSync(path.join(claudeRoot, 'ecc', 'install-state.json')), 'install state should not be written when source hooks are invalid');
} finally {
fs.writeFileSync(sourceHooksPath, originalHooks);
cleanup(homeDir);
cleanup(projectDir);
}
})) passed++; else failed++;
Comment on lines +471 to +493
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Test mutates real repository source file without crash-safe isolation

This test writes [] directly to REPO_ROOT/hooks/hooks.json — the actual committed source file — before running the installer subprocess. The finally block restores the original content after a graceful exit, but if the Node.js process is killed with SIGKILL (e.g., OOM killer, forced CI timeout, kill -9) between lines 478 and 489, the repository's hooks/hooks.json will be left as []\n permanently. Any subsequent test run or install would then fail with "Invalid hooks config".

Because the installer is invoked as a child process (execFileSync), an unhandled exception in the child cannot corrupt the parent's finally block — but a SIGKILL to the parent process absolutely can.

A safer pattern is to use a temporary directory to stage a modified copy of the hooks source and point the installer at it via an environment variable or a plan fixture, keeping the real source file untouched:

// Instead of modifying REPO_ROOT/hooks/hooks.json in-place:
const fakeSourceDir = createTempDir('fake-hooks-source-');
const fakeHooksPath = path.join(fakeSourceDir, 'hooks', 'hooks.json');
fs.mkdirSync(path.dirname(fakeHooksPath), { recursive: true });
fs.writeFileSync(fakeHooksPath, '[]\n');
// Pass fakeSourceDir to the installer via env / config
// ...
cleanup(fakeSourceDir);

Alternatively, copying the entire hooks source tree to a temp fixture directory for the test suite would eliminate the dependency on the live repo file entirely.


if (test('installs from ecc-install.json and persists component selections', () => {
const homeDir = createTempDir('install-apply-home-');
const projectDir = createTempDir('install-apply-project-');
Expand Down
Loading