From 84de0cc76029f227bd2bb02fd8705f1682966024 Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden Date: Sun, 5 Apr 2026 10:37:27 -0500 Subject: [PATCH] fix(install): comprehensive audit cleanup of hook copy, uninstall, and manifest (#1755) (#1768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add chmod +x for .sh hooks during install (fixes #1755 permission denied) - Fix Codex hook: wrong path (get-shit-done/hooks/) and inverted filename (gsd-update-check.js → gsd-check-update.js) - Fix cache invalidation path from ~/cache/ to ~/.cache/gsd/ - Track .sh hooks in writeManifest so saveLocalPatches detects modifications - Add gsd-workflow-guard.js to uninstall file cleanup list - Add community hooks (session-state, validate-commit, phase-boundary) to uninstall settings.json cleanup - Remove phantom gsd-check-update.sh from uninstall list - Remove dead isCursor/isWindsurf branches in uninstall (already handled by combined branch) - Warn when expected .sh hooks are missing after verifyInstalled - Add 15 regression tests in install-hooks-copy.test.cjs - Update codex-config.test.cjs assertions for corrected hook filename Fixes #1755 --- .plans/1755-install-audit-fix.md | 46 +++++ bin/install.js | 63 ++---- tests/codex-config.test.cjs | 42 ++-- tests/install-hooks-copy.test.cjs | 317 ++++++++++++++++++++++++++++++ 4 files changed, 404 insertions(+), 64 deletions(-) create mode 100644 .plans/1755-install-audit-fix.md create mode 100644 tests/install-hooks-copy.test.cjs diff --git a/.plans/1755-install-audit-fix.md b/.plans/1755-install-audit-fix.md new file mode 100644 index 00000000..0bc611a4 --- /dev/null +++ b/.plans/1755-install-audit-fix.md @@ -0,0 +1,46 @@ +# Plan: Fix Install Process Issues (#1755 + Full Audit) + +## Overview +Full cleanup of install.js addressing all issues found during comprehensive audit. +All changes in `bin/install.js` unless noted. + +## Changes + +### Fix 1: Add chmod +x for .sh hooks during install (CRITICAL) +**Line 5391-5392** — After `fs.copyFileSync`, add `fs.chmodSync(destFile, 0o755)` for `.sh` files. + +### Fix 2: Fix Codex hook path and filename (CRITICAL) +**Line 5485** — Change `gsd-update-check.js` to `gsd-check-update.js` and fix path from `get-shit-done/hooks/` to `hooks/`. +**Line 5492** — Update dedup check to use `gsd-check-update`. + +### Fix 3: Fix stale cache invalidation path (CRITICAL) +**Line 5406** — Change from `path.join(path.dirname(targetDir), 'cache', ...)` to `path.join(os.homedir(), '.cache', 'gsd', 'gsd-update-check.json')`. + +### Fix 4: Track .sh hooks in manifest (MEDIUM) +**Line 4972** — Change filter from `file.endsWith('.js')` to `(file.endsWith('.js') || file.endsWith('.sh'))`. + +### Fix 5: Add gsd-workflow-guard.js to uninstall hook list (MEDIUM) +**Line 4404** — Add `'gsd-workflow-guard.js'` to the `gsdHooks` array. + +### Fix 6: Add community hooks to uninstall settings.json cleanup (MEDIUM) +**Lines 4453-4520** — Add filters for `gsd-session-state`, `gsd-validate-commit`, `gsd-phase-boundary` in the appropriate event cleanup blocks (SessionStart, PreToolUse, PostToolUse). + +### Fix 7: Remove phantom gsd-check-update.sh from uninstall list (LOW) +**Line 4404** — Remove `'gsd-check-update.sh'` from `gsdHooks` array. + +### Fix 8: Remove dead isCursor/isWindsurf branches in uninstall (LOW) +Remove the unreachable duplicate `else if (isCursor)` and `else if (isWindsurf)` branches. + +### Fix 9: Improve verifyInstalled() for hooks (LOW) +After the generic check, warn if expected `.sh` files are missing (non-fatal warning). + +## New Test File +`tests/install-hooks-copy.test.cjs` — Regression tests covering: +- .sh files copied to target dir +- .sh files are executable after copy +- .sh files tracked in manifest +- settings.json hook paths match installed files +- uninstall removes community hooks from settings.json +- uninstall removes gsd-workflow-guard.js +- Codex hook uses correct filename +- Cache path resolves correctly diff --git a/bin/install.js b/bin/install.js index 20664f5e..5497b61e 100755 --- a/bin/install.js +++ b/bin/install.js @@ -4260,40 +4260,6 @@ function uninstall(isGlobal, runtime = 'claude') { console.log(` ${green}✓${reset} Removed ${skillCount} Antigravity skills`); } } - } else if (isCursor) { - // Cursor: remove skills/gsd-*/ directories (same layout as Codex skills) - const skillsDir = path.join(targetDir, 'skills'); - if (fs.existsSync(skillsDir)) { - let skillCount = 0; - const entries = fs.readdirSync(skillsDir, { withFileTypes: true }); - for (const entry of entries) { - if (entry.isDirectory() && entry.name.startsWith('gsd-')) { - fs.rmSync(path.join(skillsDir, entry.name), { recursive: true }); - skillCount++; - } - } - if (skillCount > 0) { - removedCount++; - console.log(` ${green}✓${reset} Removed ${skillCount} Cursor skills`); - } - } - } else if (isWindsurf) { - // Windsurf: remove skills/gsd-*/ directories (same layout as Cursor skills) - const skillsDir = path.join(targetDir, 'skills'); - if (fs.existsSync(skillsDir)) { - let skillCount = 0; - const entries = fs.readdirSync(skillsDir, { withFileTypes: true }); - for (const entry of entries) { - if (entry.isDirectory() && entry.name.startsWith('gsd-')) { - fs.rmSync(path.join(skillsDir, entry.name), { recursive: true }); - skillCount++; - } - } - if (skillCount > 0) { - removedCount++; - console.log(` ${green}✓${reset} Removed ${skillCount} Windsurf skills`); - } - } } else if (isGemini) { // Gemini: still uses commands/gsd/ const gsdCommandsDir = path.join(targetDir, 'commands', 'gsd'); @@ -4403,7 +4369,7 @@ function uninstall(isGlobal, runtime = 'claude') { // 4. Remove GSD hooks const hooksDir = path.join(targetDir, 'hooks'); if (fs.existsSync(hooksDir)) { - const gsdHooks = ['gsd-statusline.js', 'gsd-check-update.js', 'gsd-check-update.sh', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', 'gsd-read-guard.js', 'gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh']; + const gsdHooks = ['gsd-statusline.js', 'gsd-check-update.js', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', 'gsd-read-guard.js', 'gsd-workflow-guard.js', 'gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh']; let hookCount = 0; for (const hook of gsdHooks) { const hookPath = path.join(hooksDir, hook); @@ -4459,7 +4425,7 @@ function uninstall(isGlobal, runtime = 'claude') { if (entry.hooks && Array.isArray(entry.hooks)) { // Filter out GSD hooks const hasGsdHook = entry.hooks.some(h => - h.command && (h.command.includes('gsd-check-update') || h.command.includes('gsd-statusline')) + h.command && (h.command.includes('gsd-check-update') || h.command.includes('gsd-statusline') || h.command.includes('gsd-session-state')) ); return !hasGsdHook; } @@ -4482,7 +4448,7 @@ function uninstall(isGlobal, runtime = 'claude') { settings.hooks[eventName] = settings.hooks[eventName].filter(entry => { if (entry.hooks && Array.isArray(entry.hooks)) { const hasGsdHook = entry.hooks.some(h => - h.command && h.command.includes('gsd-context-monitor') + h.command && (h.command.includes('gsd-context-monitor') || h.command.includes('gsd-phase-boundary')) ); return !hasGsdHook; } @@ -4505,7 +4471,7 @@ function uninstall(isGlobal, runtime = 'claude') { settings.hooks[eventName] = settings.hooks[eventName].filter(entry => { if (entry.hooks && Array.isArray(entry.hooks)) { const hasGsdHook = entry.hooks.some(h => - h.command && (h.command.includes('gsd-prompt-guard') || h.command.includes('gsd-read-guard')) + h.command && (h.command.includes('gsd-prompt-guard') || h.command.includes('gsd-read-guard') || h.command.includes('gsd-validate-commit')) ); return !hasGsdHook; } @@ -4971,7 +4937,7 @@ function writeManifest(configDir, runtime = 'claude') { const hooksDir = path.join(configDir, 'hooks'); if (fs.existsSync(hooksDir)) { for (const file of fs.readdirSync(hooksDir)) { - if (file.startsWith('gsd-') && file.endsWith('.js')) { + if (file.startsWith('gsd-') && (file.endsWith('.js') || file.endsWith('.sh'))) { manifest.files['hooks/' + file] = fileHash(path.join(hooksDir, file)); } } @@ -5392,11 +5358,22 @@ function install(isGlobal, runtime = 'claude') { try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows doesn't support chmod */ } } else { fs.copyFileSync(srcFile, destFile); + // Ensure .sh hook files are executable (mirrors chmod in build-hooks.js) + if (entry.endsWith('.sh')) { + try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows doesn't support chmod */ } + } } } } if (verifyInstalled(hooksDest, 'hooks')) { console.log(` ${green}✓${reset} Installed hooks (bundled)`); + // Warn if expected community .sh hooks are missing (non-fatal) + const expectedShHooks = ['gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh']; + for (const sh of expectedShHooks) { + if (!fs.existsSync(path.join(hooksDest, sh))) { + console.warn(` ${yellow}⚠${reset} Missing expected hook: ${sh}`); + } + } } else { failures.push('hooks'); } @@ -5404,8 +5381,8 @@ function install(isGlobal, runtime = 'claude') { } // Clear stale update cache so next session re-evaluates hook versions - // targetDir is e.g. ~/.claude/get-shit-done/, parent is the config dir - const updateCacheFile = path.join(path.dirname(targetDir), 'cache', 'gsd-update-check.json'); + // Cache lives at ~/.cache/gsd/ (see hooks/gsd-check-update.js line 35-36) + const updateCacheFile = path.join(os.homedir(), '.cache', 'gsd', 'gsd-update-check.json'); try { fs.unlinkSync(updateCacheFile); } catch (e) { /* cache may not exist yet */ } if (failures.length > 0) { @@ -5484,14 +5461,14 @@ function install(isGlobal, runtime = 'claude') { configContent = setManagedCodexHooksOwnership(codexHooksFeature.content, codexHooksFeature.ownership); // Add SessionStart hook for update checking - const updateCheckScript = path.resolve(targetDir, 'get-shit-done', 'hooks', 'gsd-update-check.js').replace(/\\/g, '/'); + const updateCheckScript = path.resolve(targetDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); const hookBlock = `${eol}# GSD Hooks${eol}` + `[[hooks]]${eol}` + `event = "SessionStart"${eol}` + `command = "node ${updateCheckScript}"${eol}`; - if (hasEnabledCodexHooksFeature(configContent) && !configContent.includes('gsd-update-check')) { + if (hasEnabledCodexHooksFeature(configContent) && !configContent.includes('gsd-check-update')) { configContent += hookBlock; } diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 2f5f04ea..ab95c1a9 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -756,7 +756,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(content.includes('[features]\ncodex_hooks = true\n'), 'writes codex_hooks feature'); assert.ok(content.includes('# GSD Hooks\n[[hooks]]\nevent = "SessionStart"\n'), 'writes GSD SessionStart hook block'); assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'writes one codex_hooks key'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'writes one GSD update hook'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'writes one GSD update hook'); assertNoDraftRootKeys(content); assertUsesOnlyEol(content, '\n'); }); @@ -838,7 +838,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(content.includes('# user comment'), 'preserves user comment'); assert.ok(content.includes('[model]\nname = "o3"'), 'preserves model section'); assert.ok(content.includes('command = "echo custom"'), 'preserves custom hook'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'adds one GSD update hook'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'adds one GSD update hook'); assertNoDraftRootKeys(content); }); @@ -975,7 +975,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(!content.includes('codex_hooks = false'), 'removes false codex_hooks value'); assert.ok(content.includes('other_feature = true'), 'preserves other feature keys'); assert.ok(content.includes('command = "echo custom"'), 'preserves custom hook'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'does not duplicate GSD update hook'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'does not duplicate GSD update hook'); assertNoDraftRootKeys(content); }); @@ -1017,7 +1017,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^"codex_hooks" = true$/gm), 1, 'normalizes the quoted codex_hooks key to true'); assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 0, 'does not prepend a second bare features table'); assert.ok(content.includes('other_feature = true'), 'preserves existing feature keys'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'keeps one GSD update hook'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'keeps one GSD update hook'); assertNoDraftRootKeys(content); }); @@ -1038,7 +1038,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(content.includes('[features."a#b"]\nenabled = true'), 'preserves the quoted nested features table'); assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'adds one real top-level features table'); assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds one codex_hooks key'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'remains idempotent for the GSD hook block'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); assertNoDraftRootKeys(content); }); @@ -1058,7 +1058,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 0, 'does not add a [features] table'); assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 1, 'adds one dotted codex_hooks key'); assert.ok(content.includes('features.other_feature = true'), 'preserves existing dotted features key'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'adds one GSD update hook for dotted codex_hooks and remains idempotent'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'adds one GSD update hook for dotted codex_hooks and remains idempotent'); assertNoDraftRootKeys(content); }); @@ -1078,7 +1078,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(content.includes('features = { other_feature = true }'), 'preserves the root inline-table assignment'); assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 0, 'does not append an invalid dotted codex_hooks key'); assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 0, 'does not prepend a features table'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 0, 'does not add the GSD hook block when codex_hooks cannot be enabled safely'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 0, 'does not add the GSD hook block when codex_hooks cannot be enabled safely'); assert.ok(content.includes('[agents.gsd-executor]'), 'still installs the managed agent block'); assertNoDraftRootKeys(content); }); @@ -1099,7 +1099,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(content.includes('features = "disabled"'), 'preserves the root scalar assignment'); assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 0, 'does not append an invalid dotted codex_hooks key'); assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 0, 'does not prepend a features table'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 0, 'does not add the GSD hook block when codex_hooks cannot be enabled safely'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 0, 'does not add the GSD hook block when codex_hooks cannot be enabled safely'); assert.ok(content.includes('[agents.gsd-executor]'), 'still installs the managed agent block'); assertNoDraftRootKeys(content); }); @@ -1122,7 +1122,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^features\."codex_hooks" = true$/gm), 1, 'normalizes the quoted dotted key to true'); assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 0, 'does not append a bare dotted duplicate'); assert.ok(content.includes('features.other_feature = true'), 'preserves other dotted features keys'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'adds one GSD update hook for quoted dotted codex_hooks and remains idempotent'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'adds one GSD update hook for quoted dotted codex_hooks and remains idempotent'); assertNoDraftRootKeys(content); }); @@ -1230,7 +1230,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'replaces the multiline basic-string assignment with one true value'); assert.ok(!content.includes('multiline-basic-sentinel'), 'removes multiline basic-string continuation lines'); assert.ok(content.includes('other_feature = true'), 'preserves following feature keys'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'remains idempotent for the GSD hook block'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); assertNoDraftRootKeys(content); }); @@ -1255,7 +1255,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'replaces the multiline literal-string assignment with one true value'); assert.ok(!content.includes('multiline-literal-sentinel'), 'removes multiline literal-string continuation lines'); assert.ok(content.includes('other_feature = true'), 'preserves following feature keys'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'remains idempotent for the GSD hook block'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); assertNoDraftRootKeys(content); }); @@ -1281,7 +1281,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(!content.includes('array-sentinel-1'), 'removes multiline array continuation lines'); assert.ok(!content.includes('array-sentinel-2'), 'removes multiline array continuation lines'); assert.ok(content.includes('other_feature = true'), 'preserves following feature keys'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'remains idempotent for the GSD hook block'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); assertNoDraftRootKeys(content); }); @@ -1326,7 +1326,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'keeps one codex_hooks = true'); assert.ok(content.includes('other_feature = true'), 'preserves other feature keys'); assert.strictEqual(countMatches(content, /echo custom-after-command/g), 1, 'preserves non-GSD hook exactly once'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'keeps one GSD update hook'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'keeps one GSD update hook'); assertUsesOnlyEol(content, '\r\n'); assertNoDraftRootKeys(content); }); @@ -1349,7 +1349,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'keeps one [features] section'); assert.strictEqual(countMatches(content, /^codex_hooks = true # keep me$/gm), 1, 'preserves the commented true value'); assert.ok(content.includes('other_feature = true'), 'preserves other feature keys'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'adds the GSD update hook once'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'adds the GSD update hook once'); assertNoDraftRootKeys(content); }); @@ -1366,7 +1366,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(content.includes('# GSD Hooks\n[[hooks]]\nevent = "SessionStart"\n'), 'writes the GSD hook block using the first newline style'); assert.ok(content.includes('[model]\r\nname = "o3"'), 'preserves the existing CRLF model lines'); assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'remains idempotent on repeated installs'); - assert.strictEqual(countMatches(content, /gsd-update-check\.js/g), 1, 'does not duplicate the GSD hook block'); + assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'does not duplicate the GSD hook block'); assertNoDraftRootKeys(content); }); }); @@ -1415,7 +1415,7 @@ describe('Codex uninstall symmetry for hook-enabled configs', () => { assert.ok(cleaned.includes('# keep me'), 'preserves user comments in [features]'); assert.ok(cleaned.includes('other_feature = true'), 'preserves other feature keys'); assert.strictEqual(countMatches(cleaned, /echo custom-after-command/g), 1, 'preserves non-GSD hooks'); - assert.strictEqual(countMatches(cleaned, /gsd-update-check\.js/g), 0, 'removes only the GSD update hook'); + assert.strictEqual(countMatches(cleaned, /gsd-check-update\.js/g), 0, 'removes only the GSD update hook'); assert.strictEqual(countMatches(cleaned, /\[agents\.gsd-/g), 0, 'removes managed GSD agent sections'); assertUsesOnlyEol(cleaned, '\r\n'); }); @@ -1440,7 +1440,7 @@ describe('Codex uninstall symmetry for hook-enabled configs', () => { assert.strictEqual(countMatches(cleaned, /^features\.codex_hooks = true$/gm), 0, 'removes the dotted GSD codex_hooks key'); assert.strictEqual(countMatches(cleaned, /^\[features\]\s*$/gm), 0, 'does not leave behind a [features] table'); assert.strictEqual(countMatches(cleaned, /echo custom-after-command/g), 1, 'preserves non-GSD hooks'); - assert.strictEqual(countMatches(cleaned, /gsd-update-check\.js/g), 0, 'removes the GSD update hook'); + assert.strictEqual(countMatches(cleaned, /gsd-check-update\.js/g), 0, 'removes the GSD update hook'); }); test('install then uninstall preserves a pre-existing [features].codex_hooks = true', () => { @@ -1459,7 +1459,7 @@ describe('Codex uninstall symmetry for hook-enabled configs', () => { const cleaned = stripGsdFromCodexConfig(readCodexConfig(codexHome)); assert.ok(cleaned.includes('[features]\ncodex_hooks = true\nother_feature = true'), 'preserves the user-authored codex_hooks assignment'); assert.strictEqual(countMatches(cleaned, /^codex_hooks = true$/gm), 1, 'keeps the pre-existing codex_hooks key'); - assert.strictEqual(countMatches(cleaned, /gsd-update-check\.js/g), 0, 'removes the GSD update hook'); + assert.strictEqual(countMatches(cleaned, /gsd-check-update\.js/g), 0, 'removes the GSD update hook'); assert.strictEqual(countMatches(cleaned, /\[agents\.gsd-/g), 0, 'removes managed GSD agent sections'); }); @@ -1479,7 +1479,7 @@ describe('Codex uninstall symmetry for hook-enabled configs', () => { const cleaned = stripGsdFromCodexConfig(readCodexConfig(codexHome)); assert.ok(cleaned.includes('[features]\n"codex_hooks" = true\nother_feature = true'), 'preserves the user-authored quoted codex_hooks assignment'); assert.strictEqual(countMatches(cleaned, /^"codex_hooks" = true$/gm), 1, 'keeps the pre-existing quoted codex_hooks key'); - assert.strictEqual(countMatches(cleaned, /gsd-update-check\.js/g), 0, 'removes the GSD update hook'); + assert.strictEqual(countMatches(cleaned, /gsd-check-update\.js/g), 0, 'removes the GSD update hook'); assert.strictEqual(countMatches(cleaned, /\[agents\.gsd-/g), 0, 'removes managed GSD agent sections'); }); @@ -1498,7 +1498,7 @@ describe('Codex uninstall symmetry for hook-enabled configs', () => { const cleaned = stripGsdFromCodexConfig(readCodexConfig(codexHome)); assert.ok(cleaned.includes('features.codex_hooks = true\nfeatures.other_feature = true'), 'preserves the user-authored dotted codex_hooks assignment'); assert.strictEqual(countMatches(cleaned, /^features\.codex_hooks = true$/gm), 1, 'keeps the pre-existing dotted codex_hooks key'); - assert.strictEqual(countMatches(cleaned, /gsd-update-check\.js/g), 0, 'removes the GSD update hook'); + assert.strictEqual(countMatches(cleaned, /gsd-check-update\.js/g), 0, 'removes the GSD update hook'); assert.strictEqual(countMatches(cleaned, /\[agents\.gsd-/g), 0, 'removes managed GSD agent sections'); }); @@ -1537,7 +1537,7 @@ describe('Codex uninstall symmetry for hook-enabled configs', () => { const cleaned = stripGsdFromCodexConfig(readCodexConfig(codexHome)); assert.ok(cleaned.includes('# first line wins\n[features]\r\nother_feature = true\r\n\r\n[model]\r\nname = "o3"'), 'preserves the original mixed-EOL user content'); assert.strictEqual(countMatches(cleaned, /^codex_hooks = true$/gm), 0, 'removes the injected codex_hooks key'); - assert.strictEqual(countMatches(cleaned, /gsd-update-check\.js/g), 0, 'removes the GSD update hook'); + assert.strictEqual(countMatches(cleaned, /gsd-check-update\.js/g), 0, 'removes the GSD update hook'); assert.strictEqual(countMatches(cleaned, /\[agents\.gsd-/g), 0, 'removes managed GSD agent sections'); }); }); diff --git a/tests/install-hooks-copy.test.cjs b/tests/install-hooks-copy.test.cjs new file mode 100644 index 00000000..d8b39504 --- /dev/null +++ b/tests/install-hooks-copy.test.cjs @@ -0,0 +1,317 @@ +/** + * Regression tests for install process hook copying, permissions, manifest + * tracking, uninstall cleanup, and settings.json registration. + * + * Covers: #1755, Codex hook path/filename, cache invalidation path, + * manifest .sh tracking, uninstall settings cleanup, dead code removal. + */ + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { test, describe, beforeEach, afterEach, before } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const { execFileSync } = require('child_process'); +const { cleanup, createTempDir } = require('./helpers.cjs'); + +const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js'); +const { writeManifest } = require(INSTALL_SRC); +const BUILD_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); +const HOOKS_DIST = path.join(__dirname, '..', 'hooks', 'dist'); + +// Expected .sh community hooks +const EXPECTED_SH_HOOKS = [ + 'gsd-session-state.sh', + 'gsd-validate-commit.sh', + 'gsd-phase-boundary.sh', +]; + +// All hooks that should be in hooks/dist/ after build +const EXPECTED_ALL_HOOKS = [ + 'gsd-check-update.js', + 'gsd-context-monitor.js', + 'gsd-prompt-guard.js', + 'gsd-read-guard.js', + 'gsd-statusline.js', + 'gsd-workflow-guard.js', + ...EXPECTED_SH_HOOKS, +]; + +const isWindows = process.platform === 'win32'; + +// ─── Ensure hooks/dist/ is populated ──────────────────────────────────────── + +before(() => { + execFileSync(process.execPath, [BUILD_SCRIPT], { + encoding: 'utf-8', + stdio: 'pipe', + }); +}); + +// ─── Helper: simulate the hook copy loop from install.js ──────────────────── +// NOTE: This helper mirrors the chmod/copy logic only. It omits the .js +// template substitution ('.claude' → runtime dir, {{GSD_VERSION}} stamping) +// since these tests focus on file presence and permissions, not content. + +function simulateHookCopy(hooksSrc, hooksDest) { + fs.mkdirSync(hooksDest, { recursive: true }); + const hookEntries = fs.readdirSync(hooksSrc); + for (const entry of hookEntries) { + const srcFile = path.join(hooksSrc, entry); + if (fs.statSync(srcFile).isFile()) { + const destFile = path.join(hooksDest, entry); + if (entry.endsWith('.js')) { + const content = fs.readFileSync(srcFile, 'utf8'); + fs.writeFileSync(destFile, content); + try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows */ } + } else { + fs.copyFileSync(srcFile, destFile); + if (entry.endsWith('.sh')) { + try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows */ } + } + } + } + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// 1. Hook file copy and permissions (#1755) +// ───────────────────────────────────────────────────────────────────────────── + +describe('#1755: .sh hooks are copied and executable after install', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempDir('gsd-hook-copy-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('all expected hooks are copied from hooks/dist/ to target', () => { + const hooksDest = path.join(tmpDir, 'hooks'); + simulateHookCopy(HOOKS_DIST, hooksDest); + + for (const hook of EXPECTED_ALL_HOOKS) { + assert.ok( + fs.existsSync(path.join(hooksDest, hook)), + `${hook} should exist in target hooks dir` + ); + } + }); + + test('.sh hooks are executable after copy', { + skip: isWindows ? 'Windows has no POSIX file permissions' : false, + }, () => { + const hooksDest = path.join(tmpDir, 'hooks'); + simulateHookCopy(HOOKS_DIST, hooksDest); + + for (const sh of EXPECTED_SH_HOOKS) { + const stat = fs.statSync(path.join(hooksDest, sh)); + assert.ok( + (stat.mode & 0o111) !== 0, + `${sh} should be executable after install copy` + ); + } + }); + + test('.js hooks are executable after copy', { + skip: isWindows ? 'Windows has no POSIX file permissions' : false, + }, () => { + const hooksDest = path.join(tmpDir, 'hooks'); + simulateHookCopy(HOOKS_DIST, hooksDest); + + const jsHooks = EXPECTED_ALL_HOOKS.filter(h => h.endsWith('.js')); + for (const js of jsHooks) { + const stat = fs.statSync(path.join(hooksDest, js)); + assert.ok( + (stat.mode & 0o111) !== 0, + `${js} should be executable after install copy` + ); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 2. install.js source-level correctness checks +// ───────────────────────────────────────────────────────────────────────────── + +describe('install.js source correctness', () => { + let src; + + before(() => { + src = fs.readFileSync(INSTALL_SRC, 'utf-8'); + }); + + test('.sh files get chmod after copyFileSync', () => { + // The else branch for non-.js hooks should apply chmod for .sh files + assert.ok( + src.includes("if (entry.endsWith('.sh'))"), + 'install.js should check for .sh extension to apply chmod' + ); + }); + + test('Codex hook uses correct filename gsd-check-update.js (not gsd-update-check.js)', () => { + // The cache file gsd-update-check.json is legitimate (different artifact); + // check that no hook registration uses the inverted .js filename. + // Match the exact pattern: quote + gsd-update-check.js + quote + assert.ok( + !src.match(/['"]gsd-update-check\.js['"]/), + 'install.js must not reference the inverted hook name gsd-update-check.js in quotes' + ); + }); + + test('Codex hook path does not use get-shit-done/hooks/ subdirectory', () => { + // The Codex hook should resolve to targetDir/hooks/, not targetDir/get-shit-done/hooks/ + assert.ok( + !src.includes("'get-shit-done', 'hooks', 'gsd-check-update"), + 'Codex hook should not use get-shit-done/hooks/ path segment' + ); + }); + + test('cache invalidation uses ~/.cache/gsd/ path', () => { + assert.ok( + src.includes("os.homedir(), '.cache', 'gsd'"), + 'Cache path should use os.homedir()/.cache/gsd/' + ); + }); + + test('manifest tracks .sh hook files', () => { + assert.ok( + src.includes("file.endsWith('.sh')"), + 'writeManifest should track .sh files in addition to .js' + ); + }); + + test('gsd-workflow-guard.js is in uninstall hook list', () => { + const gsdHooksMatch = src.match(/const gsdHooks\s*=\s*\[([^\]]+)\]/); + assert.ok(gsdHooksMatch, 'gsdHooks array should exist'); + const gsdHooksContent = gsdHooksMatch[1]; + assert.ok( + gsdHooksContent.includes('gsd-workflow-guard.js'), + 'gsdHooks should include gsd-workflow-guard.js' + ); + }); + + test('phantom gsd-check-update.sh is not in uninstall hook list', () => { + const gsdHooksMatch = src.match(/const gsdHooks\s*=\s*\[([^\]]+)\]/); + assert.ok(gsdHooksMatch, 'gsdHooks array should exist'); + const gsdHooksContent = gsdHooksMatch[1]; + assert.ok( + !gsdHooksContent.includes('gsd-check-update.sh'), + 'gsdHooks should not include phantom gsd-check-update.sh' + ); + }); + + test('uninstall settings cleanup includes community hooks', () => { + // SessionStart cleanup should include gsd-session-state + assert.ok( + src.includes("gsd-session-state"), + 'uninstall should filter gsd-session-state from SessionStart' + ); + + // PostToolUse/AfterTool cleanup should include gsd-phase-boundary + const postStart = src.indexOf('Remove GSD hooks from PostToolUse'); + const postEnd = src.indexOf('Remove GSD hooks from PreToolUse'); + assert.ok(postStart !== -1, 'PostToolUse cleanup marker must exist'); + assert.ok(postEnd !== -1, 'PreToolUse cleanup marker must exist'); + const postToolBlock = src.substring(postStart, postEnd); + assert.ok( + postToolBlock.includes('gsd-phase-boundary'), + 'uninstall should filter gsd-phase-boundary from PostToolUse/AfterTool' + ); + + // PreToolUse/BeforeTool cleanup should include gsd-validate-commit + const preStart = src.indexOf('Remove GSD hooks from PreToolUse'); + const preEnd = src.indexOf('Clean up empty hooks object'); + assert.ok(preStart !== -1, 'PreToolUse cleanup marker must exist'); + assert.ok(preEnd !== -1, 'empty hooks cleanup marker must exist'); + const preToolBlock = src.substring(preStart, preEnd); + assert.ok( + preToolBlock.includes('gsd-validate-commit'), + 'uninstall should filter gsd-validate-commit from PreToolUse/BeforeTool' + ); + }); + + test('no duplicate isCursor or isWindsurf branches in uninstall skill removal', () => { + // The uninstall skill removal if/else chain should not have standalone + // isCursor or isWindsurf branches — they're already handled by the combined + // (isCodex || isCursor || isWindsurf || isTrae) branch + const uninstallStart = src.indexOf('function uninstall('); + const uninstallEnd = src.indexOf('function verifyInstalled('); + assert.ok(uninstallStart !== -1, 'function uninstall( must exist in install.js'); + assert.ok(uninstallEnd !== -1, 'function verifyInstalled( must exist in install.js'); + const uninstallBlock = src.substring(uninstallStart, uninstallEnd); + + // Count occurrences of 'else if (isCursor)' in uninstall — should be 0 + const cursorBranches = (uninstallBlock.match(/else if \(isCursor\)/g) || []).length; + assert.strictEqual(cursorBranches, 0, 'No standalone isCursor branch should exist in uninstall'); + + // Count occurrences of 'else if (isWindsurf)' in uninstall — should be 0 + const windsurfBranches = (uninstallBlock.match(/else if \(isWindsurf\)/g) || []).length; + assert.strictEqual(windsurfBranches, 0, 'No standalone isWindsurf branch should exist in uninstall'); + }); + + test('verifyInstalled warns about missing .sh hooks', () => { + assert.ok( + src.includes('Missing expected hook:'), + 'install should warn about missing .sh hooks after verification' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 3. Manifest tracks .sh hooks +// ───────────────────────────────────────────────────────────────────────────── + +describe('writeManifest includes .sh hooks', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempDir('gsd-manifest-'); + // Set up minimal structure expected by writeManifest + const hooksDir = path.join(tmpDir, 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + // Copy hooks from dist to simulate install + simulateHookCopy(HOOKS_DIST, hooksDir); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('manifest contains .sh hook entries', () => { + writeManifest(tmpDir, 'claude'); + + const manifestPath = path.join(tmpDir, 'gsd-file-manifest.json'); + assert.ok(fs.existsSync(manifestPath), 'manifest file should exist'); + + const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); + + for (const sh of EXPECTED_SH_HOOKS) { + assert.ok( + manifest.files['hooks/' + sh], + `manifest should contain hash for ${sh}` + ); + } + }); + + test('manifest contains .js hook entries', () => { + writeManifest(tmpDir, 'claude'); + + const manifestPath = path.join(tmpDir, 'gsd-file-manifest.json'); + const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); + + const jsHooks = EXPECTED_ALL_HOOKS.filter(h => h.endsWith('.js')); + for (const js of jsHooks) { + assert.ok( + manifest.files['hooks/' + js], + `manifest should contain hash for ${js}` + ); + } + }); +});