From dba5ae779b7347302108f5af7ebdefc7d3fa25f8 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Wed, 1 Apr 2026 16:04:56 -0700 Subject: [PATCH] fix: harden install and codex sync portability --- WORKING-CONTEXT.md | 5 ++ scripts/lib/install/apply.js | 69 ++++++++++++++++++++----- scripts/sync-ecc-to-codex.sh | 18 ++++++- tests/scripts/install-apply.test.js | 39 ++++++++++++++ tests/scripts/sync-ecc-to-codex.test.js | 9 ++++ 5 files changed, 125 insertions(+), 15 deletions(-) diff --git a/WORKING-CONTEXT.md b/WORKING-CONTEXT.md index 7d42e2b5..574cd572 100644 --- a/WORKING-CONTEXT.md +++ b/WORKING-CONTEXT.md @@ -59,6 +59,8 @@ Public ECC plugin repo for agents, skills, commands, hooks, rules, install surfa - Direct-port candidates landed after audit: - `#1078` hook-id dedupe for managed Claude hook reinstalls - `#844` ui-demo skill + - `#1110` install-time Claude hook root resolution + - `#1106` portable Codex Context7 key extraction - Port or rebuild inside ECC after full audit: - `#894` Jira integration - `#814` + `#808` rebuild as a single consolidated notifications lane for Opencode and cross-harness surfaces @@ -97,3 +99,6 @@ Keep this file detailed for only the current sprint, blockers, and next actions. - 2026-04-01: Collapsed the obvious command/skill duplicates into thin legacy shims so `skills/` now hold the maintained bodies for NanoClaw, context-budget, DevFleet, docs lookup, E2E, evals, orchestration, prompt optimization, rules distillation, TDD, and verification. - 2026-04-01: Ported the self-contained core of `#844` directly into `main` as `skills/ui-demo/SKILL.md` and registered it under the `media-generation` install module instead of merging the PR wholesale. - 2026-04-01: Added the first connected-workflow operator lane as ECC-native skills instead of leaving the surface as raw plugins or APIs: `workspace-surface-audit`, `customer-billing-ops`, `project-flow-ops`, and `google-workspace-ops`. These are tracked under the new `operator-workflows` install module. +- 2026-04-01: Direct-ported the real fix from the unresolved hook-path PR lane into the active installer. Claude installs now replace `${CLAUDE_PLUGIN_ROOT}` with the concrete install root in both `settings.json` and the copied `hooks/hooks.json`, which keeps PreToolUse/PostToolUse hooks working outside plugin-managed env injection. +- 2026-04-01: Replaced the GNU-only `grep -P` parser in `scripts/sync-ecc-to-codex.sh` with a portable Node parser for Context7 key extraction. Added source-level regression coverage so BSD/macOS syncs do not drift back to non-portable parsing. +- 2026-04-01: Targeted regression suite after the direct ports is green: `tests/scripts/install-apply.test.js`, `tests/scripts/sync-ecc-to-codex.test.js`, and `tests/scripts/codex-hooks.test.js`. diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 9ed7bb82..40f3b55b 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -20,16 +20,43 @@ function readJsonObject(filePath, label) { return parsed; } -function buildLegacyHookSignature(entry) { +function replacePluginRootPlaceholders(value, pluginRoot) { + if (!pluginRoot) { + return value; + } + + if (typeof value === 'string') { + return value.split('${CLAUDE_PLUGIN_ROOT}').join(pluginRoot); + } + + if (Array.isArray(value)) { + return value.map(item => replacePluginRootPlaceholders(item, pluginRoot)); + } + + if (value && typeof value === 'object') { + return Object.fromEntries( + Object.entries(value).map(([key, nestedValue]) => [ + key, + replacePluginRootPlaceholders(nestedValue, pluginRoot), + ]) + ); + } + + return value; +} + +function buildLegacyHookSignature(entry, pluginRoot) { if (!entry || typeof entry !== 'object' || Array.isArray(entry)) { return null; } - if (typeof entry.matcher !== 'string' || !Array.isArray(entry.hooks)) { + const normalizedEntry = replacePluginRootPlaceholders(entry, pluginRoot); + + if (typeof normalizedEntry.matcher !== 'string' || !Array.isArray(normalizedEntry.hooks)) { return null; } - const hookSignature = entry.hooks.map(hook => JSON.stringify({ + const hookSignature = normalizedEntry.hooks.map(hook => JSON.stringify({ type: hook && typeof hook === 'object' ? hook.type : undefined, command: hook && typeof hook === 'object' ? hook.command : undefined, timeout: hook && typeof hook === 'object' ? hook.timeout : undefined, @@ -37,33 +64,35 @@ function buildLegacyHookSignature(entry) { })); return JSON.stringify({ - matcher: entry.matcher, + matcher: normalizedEntry.matcher, hooks: hookSignature, }); } -function getHookEntryAliases(entry) { +function getHookEntryAliases(entry, pluginRoot) { const aliases = []; if (!entry || typeof entry !== 'object' || Array.isArray(entry)) { return aliases; } - if (typeof entry.id === 'string' && entry.id.trim().length > 0) { - aliases.push(`id:${entry.id.trim()}`); + const normalizedEntry = replacePluginRootPlaceholders(entry, pluginRoot); + + if (typeof normalizedEntry.id === 'string' && normalizedEntry.id.trim().length > 0) { + aliases.push(`id:${normalizedEntry.id.trim()}`); } - const legacySignature = buildLegacyHookSignature(entry); + const legacySignature = buildLegacyHookSignature(normalizedEntry, pluginRoot); if (legacySignature) { aliases.push(`legacy:${legacySignature}`); } - aliases.push(`json:${JSON.stringify(entry)}`); + aliases.push(`json:${JSON.stringify(normalizedEntry)}`); return aliases; } -function mergeHookEntries(existingEntries, incomingEntries) { +function mergeHookEntries(existingEntries, incomingEntries, pluginRoot) { const mergedEntries = []; const seenEntries = new Set(); @@ -76,7 +105,7 @@ function mergeHookEntries(existingEntries, incomingEntries) { continue; } - const aliases = getHookEntryAliases(entry); + const aliases = getHookEntryAliases(entry, pluginRoot); if (aliases.some(alias => seenEntries.has(alias))) { continue; } @@ -84,7 +113,7 @@ function mergeHookEntries(existingEntries, incomingEntries) { for (const alias of aliases) { seenEntries.add(alias); } - mergedEntries.push(entry); + mergedEntries.push(replacePluginRootPlaceholders(entry, pluginRoot)); } return mergedEntries; @@ -100,6 +129,7 @@ function buildMergedSettings(plan) { return null; } + const pluginRoot = plan.targetRoot; const hooksDestinationPath = path.join(plan.targetRoot, 'hooks', 'hooks.json'); const hooksSourcePath = findHooksSourcePath(plan, hooksDestinationPath) || hooksDestinationPath; if (!fs.existsSync(hooksSourcePath)) { @@ -107,7 +137,7 @@ function buildMergedSettings(plan) { } const hooksConfig = readJsonObject(hooksSourcePath, 'hooks config'); - const incomingHooks = hooksConfig.hooks; + const incomingHooks = replacePluginRootPlaceholders(hooksConfig.hooks, pluginRoot); if (!incomingHooks || typeof incomingHooks !== 'object' || Array.isArray(incomingHooks)) { throw new Error(`Invalid hooks config at ${hooksSourcePath}: expected "hooks" to be a JSON object`); } @@ -126,7 +156,7 @@ function buildMergedSettings(plan) { 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); + mergedHooks[eventName] = mergeHookEntries(currentEntries, nextEntries, pluginRoot); } const mergedSettings = { @@ -137,6 +167,11 @@ function buildMergedSettings(plan) { return { settingsPath, mergedSettings, + hooksDestinationPath, + resolvedHooksConfig: { + ...hooksConfig, + hooks: incomingHooks, + }, }; } @@ -149,6 +184,12 @@ function applyInstallPlan(plan) { } if (mergedSettingsPlan) { + fs.mkdirSync(path.dirname(mergedSettingsPlan.hooksDestinationPath), { recursive: true }); + fs.writeFileSync( + mergedSettingsPlan.hooksDestinationPath, + JSON.stringify(mergedSettingsPlan.resolvedHooksConfig, null, 2) + '\n', + 'utf8' + ); fs.mkdirSync(path.dirname(mergedSettingsPlan.settingsPath), { recursive: true }); fs.writeFileSync( mergedSettingsPlan.settingsPath, diff --git a/scripts/sync-ecc-to-codex.sh b/scripts/sync-ecc-to-codex.sh index f9602391..a2cddb0e 100755 --- a/scripts/sync-ecc-to-codex.sh +++ b/scripts/sync-ecc-to-codex.sh @@ -106,7 +106,23 @@ extract_toml_value() { extract_context7_key() { local file="$1" - grep -oP -- '--key",[[:space:]]*"\K[^"]+' "$file" | head -n 1 || true + node - "$file" <<'EOF' +const fs = require('fs'); + +const filePath = process.argv[2]; +let source = ''; + +try { + source = fs.readFileSync(filePath, 'utf8'); +} catch { + process.exit(0); +} + +const match = source.match(/--key",\s*"([^"]+)"/); +if (match && match[1]) { + process.stdout.write(`${match[1]}\n`); +} +EOF } generate_prompt_file() { diff --git a/tests/scripts/install-apply.test.js b/tests/scripts/install-apply.test.js index 7d716568..45bd44b2 100644 --- a/tests/scripts/install-apply.test.js +++ b/tests/scripts/install-apply.test.js @@ -353,6 +353,45 @@ function runTests() { } })) passed++; else failed++; + if (test('resolves CLAUDE_PLUGIN_ROOT placeholders in installed claude hooks', () => { + 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'); + const settings = readJson(path.join(claudeRoot, 'settings.json')); + const installedHooks = readJson(path.join(claudeRoot, 'hooks', 'hooks.json')); + + const autoTmuxEntry = settings.hooks.PreToolUse.find(entry => entry.id === 'pre:bash:auto-tmux-dev'); + assert.ok(autoTmuxEntry, 'settings.json should include the auto tmux hook'); + assert.ok( + autoTmuxEntry.hooks[0].command.includes(path.join(claudeRoot, 'scripts', 'hooks', 'auto-tmux-dev.js')), + 'settings.json should use the installed Claude root for hook commands' + ); + assert.ok( + !autoTmuxEntry.hooks[0].command.includes('${CLAUDE_PLUGIN_ROOT}'), + 'settings.json should not retain CLAUDE_PLUGIN_ROOT placeholders after install' + ); + + const installedAutoTmuxEntry = installedHooks.hooks.PreToolUse.find(entry => entry.id === 'pre:bash:auto-tmux-dev'); + assert.ok(installedAutoTmuxEntry, 'hooks/hooks.json should include the auto tmux hook'); + assert.ok( + installedAutoTmuxEntry.hooks[0].command.includes(path.join(claudeRoot, 'scripts', 'hooks', 'auto-tmux-dev.js')), + 'hooks/hooks.json should use the installed Claude root for hook commands' + ); + assert.ok( + !installedAutoTmuxEntry.hooks[0].command.includes('${CLAUDE_PLUGIN_ROOT}'), + 'hooks/hooks.json should not retain CLAUDE_PLUGIN_ROOT placeholders after install' + ); + } 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-'); diff --git a/tests/scripts/sync-ecc-to-codex.test.js b/tests/scripts/sync-ecc-to-codex.test.js index 3a9281b2..2626c13c 100644 --- a/tests/scripts/sync-ecc-to-codex.test.js +++ b/tests/scripts/sync-ecc-to-codex.test.js @@ -74,6 +74,15 @@ function runTests() { assert.ok(!source.includes('run_or_echo cp -R "$skill_dir" "$dest"'), 'skill sync cp should be removed'); })) passed++; else failed++; + if (test('sync script avoids GNU-only grep -P parsing', () => { + assert.ok(!source.includes('grep -oP'), 'sync-ecc-to-codex.sh should remain portable across BSD and GNU environments'); + })) passed++; else failed++; + + if (test('extract_context7_key uses a portable parser', () => { + assert.ok(source.includes('extract_context7_key() {'), 'Expected extract_context7_key helper'); + assert.ok(source.includes('node - "$file"'), 'extract_context7_key should use Node-based parsing'); + })) passed++; else failed++; + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); }