From 47aa415b06f32f5f875c6321c0f93db9a686eaaf Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Sat, 28 Mar 2026 19:55:28 -0400 Subject: [PATCH] fix(installer): validate hooks and settings before install --- scripts/ecc.js | 0 scripts/install-apply.js | 0 scripts/lib/install/apply.js | 68 +++++++++++++++++++---------- tests/scripts/install-apply.test.js | 34 ++++++++++++++- 4 files changed, 76 insertions(+), 26 deletions(-) mode change 100644 => 100755 scripts/ecc.js mode change 100644 => 100755 scripts/install-apply.js diff --git a/scripts/ecc.js b/scripts/ecc.js old mode 100644 new mode 100755 diff --git a/scripts/install-apply.js b/scripts/install-apply.js old mode 100644 new mode 100755 diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 857a1617..bab5b6d5 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -5,6 +5,21 @@ 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(); @@ -22,39 +37,32 @@ function mergeHookEntries(existingEntries, incomingEntries) { return mergedEntries; } -function mergeHooksIntoSettings(plan) { +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; + return null; } - const hooksJsonPath = path.join(plan.targetRoot, 'hooks', 'hooks.json'); - if (!fs.existsSync(hooksJsonPath)) { - return; - } - - 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 hooksDestinationPath = path.join(plan.targetRoot, 'hooks', 'hooks.json'); + const hooksSourcePath = findHooksSourcePath(plan, hooksDestinationPath) || hooksDestinationPath; + if (!fs.existsSync(hooksSourcePath)) { + return null; } + const hooksConfig = readJsonObject(hooksSourcePath, 'hooks config'); const incomingHooks = hooksConfig.hooks; if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) { - return; + 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)) { - try { - settings = JSON.parse(fs.readFileSync(settingsPath, 'utf8')); - if (!settings || typeof settings !== 'object' || Array.isArray(settings)) { - throw new Error('root value must be a JSON object'); - } - } catch (error) { - throw new Error(`Failed to parse existing settings at ${settingsPath}: ${error.message}`); - } + settings = readJsonObject(settingsPath, 'existing settings'); } const existingHooks = settings.hooks && typeof settings.hooks === 'object' && !Array.isArray(settings.hooks) @@ -73,17 +81,29 @@ function mergeHooksIntoSettings(plan) { hooks: mergedHooks, }; - fs.mkdirSync(path.dirname(settingsPath), { recursive: true }); - fs.writeFileSync(settingsPath, JSON.stringify(mergedSettings, null, 2) + '\n', 'utf8'); + return { + settingsPath, + mergedSettings, + }; } function applyInstallPlan(plan) { + const mergedSettingsPlan = buildMergedSettings(plan); + for (const operation of plan.operations) { fs.mkdirSync(path.dirname(operation.destinationPath), { recursive: true }); fs.copyFileSync(operation.sourcePath, operation.destinationPath); } - mergeHooksIntoSettings(plan); + 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 { diff --git a/tests/scripts/install-apply.test.js b/tests/scripts/install-apply.test.js index b82d67aa..1416489c 100644 --- a/tests/scripts/install-apply.test.js +++ b/tests/scripts/install-apply.test.js @@ -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, @@ -435,6 +437,8 @@ function runTests() { 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); @@ -453,15 +457,41 @@ function runTests() { 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.ok(result.stderr.includes('root value must be a JSON object')); + 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++; + if (test('installs from ecc-install.json and persists component selections', () => { const homeDir = createTempDir('install-apply-home-'); const projectDir = createTempDir('install-apply-project-');