From 175d89efa90d1675ef3b9348461fb9beb81a8c7c Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden Date: Sun, 5 Apr 2026 10:46:40 -0500 Subject: [PATCH] =?UTF-8?q?fix(install):=20uninstall=20hook=20safety=20?= =?UTF-8?q?=E2=80=94=20per-hook=20granularity,=20legacy=20migration,=20wor?= =?UTF-8?q?kflow-guard=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three findings from Codex adversarial review of #1768: - Uninstall settings cleanup now filters at per-hook granularity instead of per-entry. User hooks that share an entry with a GSD hook are preserved instead of being removed as collateral damage. - Add gsd-workflow-guard to PreToolUse/BeforeTool uninstall settings filter so opt-in users don't get dangling references after uninstall. - Codex install now strips legacy gsd-update-check.js hook entries before appending the corrected gsd-check-update.js, preventing duplicate hooks on upgrade from affected versions. - 8 new regression tests covering per-hook filtering, legacy migration regex. Fixes #1755 --- bin/install.js | 86 +++++---------- tests/install-hooks-copy.test.cjs | 177 +++++++++++++++++++++++++----- 2 files changed, 179 insertions(+), 84 deletions(-) diff --git a/bin/install.js b/bin/install.js index 5497b61e1..06b116308 100755 --- a/bin/install.js +++ b/bin/install.js @@ -4418,73 +4418,36 @@ function uninstall(isGlobal, runtime = 'claude') { console.log(` ${green}✓${reset} Removed GSD statusline from settings`); } - // Remove GSD hooks from SessionStart - if (settings.hooks && settings.hooks.SessionStart) { - const before = settings.hooks.SessionStart.length; - settings.hooks.SessionStart = settings.hooks.SessionStart.filter(entry => { - 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.includes('gsd-session-state')) - ); - return !hasGsdHook; - } - return true; - }); - if (settings.hooks.SessionStart.length < before) { - settingsModified = true; - console.log(` ${green}✓${reset} Removed GSD hooks from settings`); - } - // Clean up empty array - if (settings.hooks.SessionStart.length === 0) { - delete settings.hooks.SessionStart; - } - } + // Remove GSD hooks from settings — per-hook granularity to preserve + // user hooks that share an entry with a GSD hook (#1755 followup) + const isGsdHookCommand = (cmd) => + cmd && (cmd.includes('gsd-check-update') || cmd.includes('gsd-statusline') || + cmd.includes('gsd-session-state') || cmd.includes('gsd-context-monitor') || + cmd.includes('gsd-phase-boundary') || cmd.includes('gsd-prompt-guard') || + cmd.includes('gsd-read-guard') || cmd.includes('gsd-validate-commit') || + cmd.includes('gsd-workflow-guard')); - // Remove GSD hooks from PostToolUse and AfterTool (Gemini uses AfterTool) - for (const eventName of ['PostToolUse', 'AfterTool']) { + for (const eventName of ['SessionStart', 'PostToolUse', 'AfterTool', 'PreToolUse', 'BeforeTool']) { if (settings.hooks && settings.hooks[eventName]) { - const before = settings.hooks[eventName].length; - 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.includes('gsd-phase-boundary')) - ); - return !hasGsdHook; - } - return true; - }); - if (settings.hooks[eventName].length < before) { + const before = JSON.stringify(settings.hooks[eventName]); + settings.hooks[eventName] = settings.hooks[eventName] + .map(entry => { + if (!entry.hooks || !Array.isArray(entry.hooks)) return null; + // Filter out individual GSD hooks, keep user hooks + entry.hooks = entry.hooks.filter(h => !isGsdHookCommand(h.command)); + return entry.hooks.length > 0 ? entry : null; + }) + .filter(Boolean); + if (JSON.stringify(settings.hooks[eventName]) !== before) { settingsModified = true; - console.log(` ${green}✓${reset} Removed context monitor hook from settings`); } if (settings.hooks[eventName].length === 0) { delete settings.hooks[eventName]; } } } - - // Remove GSD hooks from PreToolUse and BeforeTool (Gemini uses BeforeTool) - for (const eventName of ['PreToolUse', 'BeforeTool']) { - if (settings.hooks && settings.hooks[eventName]) { - const before = settings.hooks[eventName].length; - 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.includes('gsd-validate-commit')) - ); - return !hasGsdHook; - } - return true; - }); - if (settings.hooks[eventName].length < before) { - settingsModified = true; - console.log(` ${green}✓${reset} Removed GSD pre-tool guard hooks from settings`); - } - if (settings.hooks[eventName].length === 0) { - delete settings.hooks[eventName]; - } - } + if (settingsModified) { + console.log(` ${green}✓${reset} Removed GSD hooks from settings`); } // Clean up empty hooks object @@ -5468,6 +5431,13 @@ function install(isGlobal, runtime = 'claude') { `event = "SessionStart"${eol}` + `command = "node ${updateCheckScript}"${eol}`; + // Migrate legacy gsd-update-check entries from prior installs (#1755 followup) + // Remove stale hook blocks that used the inverted filename or wrong path + if (configContent.includes('gsd-update-check')) { + configContent = configContent.replace(/\n# GSD Hooks\n\[\[hooks\]\]\nevent = "SessionStart"\ncommand = "node [^\n]*gsd-update-check\.js"\n/g, '\n'); + configContent = configContent.replace(/\r\n# GSD Hooks\r\n\[\[hooks\]\]\r\nevent = "SessionStart"\r\ncommand = "node [^\r\n]*gsd-update-check\.js"\r\n/g, '\r\n'); + } + if (hasEnabledCodexHooksFeature(configContent) && !configContent.includes('gsd-check-update')) { configContent += hookBlock; } diff --git a/tests/install-hooks-copy.test.cjs b/tests/install-hooks-copy.test.cjs index d8b395041..309190c10 100644 --- a/tests/install-hooks-copy.test.cjs +++ b/tests/install-hooks-copy.test.cjs @@ -18,7 +18,7 @@ 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 { writeManifest, validateHookFields } = require(INSTALL_SRC); const BUILD_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); const HOOKS_DIST = path.join(__dirname, '..', 'hooks', 'dist'); @@ -207,33 +207,25 @@ describe('install.js source correctness', () => { ); }); - 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' - ); + test('isGsdHookCommand covers all GSD hook names', () => { + // The consolidated uninstall cleanup uses isGsdHookCommand — verify all hook names are present + const expectedHookNames = [ + 'gsd-check-update', 'gsd-statusline', 'gsd-session-state', + 'gsd-context-monitor', 'gsd-phase-boundary', 'gsd-prompt-guard', + 'gsd-read-guard', 'gsd-validate-commit', 'gsd-workflow-guard', + ]; + for (const name of expectedHookNames) { + assert.ok( + src.includes(`'${name}'`) || src.includes(`"${name}"`), + `isGsdHookCommand should match ${name}` + ); + } + }); - // 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); + test('Codex install migrates legacy gsd-update-check entries', () => { 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' + src.includes('gsd-update-check'), + 'install.js should detect legacy gsd-update-check entries for migration' ); }); @@ -315,3 +307,136 @@ describe('writeManifest includes .sh hooks', () => { } }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// 4. Uninstall per-hook granularity (#1755 followup) +// ───────────────────────────────────────────────────────────────────────────── + +describe('uninstall settings cleanup preserves user hooks', () => { + // Mirror the isGsdHookCommand logic from install.js + const isGsdHookCommand = (cmd) => + cmd && (cmd.includes('gsd-check-update') || cmd.includes('gsd-statusline') || + cmd.includes('gsd-session-state') || cmd.includes('gsd-context-monitor') || + cmd.includes('gsd-phase-boundary') || cmd.includes('gsd-prompt-guard') || + cmd.includes('gsd-read-guard') || cmd.includes('gsd-validate-commit') || + cmd.includes('gsd-workflow-guard')); + + // Simulate the per-hook filtering logic from uninstall + function filterGsdHooks(entries) { + return entries + .map(entry => { + if (!entry.hooks || !Array.isArray(entry.hooks)) return null; + entry.hooks = entry.hooks.filter(h => !isGsdHookCommand(h.command)); + return entry.hooks.length > 0 ? entry : null; + }) + .filter(Boolean); + } + + test('mixed entry with GSD + user hooks preserves user hooks', () => { + const entries = [{ + matcher: 'Bash', + hooks: [ + { type: 'command', command: 'node /path/to/gsd-prompt-guard.js' }, + { type: 'command', command: 'bash /my/custom-lint.sh' }, + ], + }]; + + const result = filterGsdHooks(entries); + assert.strictEqual(result.length, 1, 'entry should survive with remaining user hook'); + assert.strictEqual(result[0].hooks.length, 1, 'only user hook should remain'); + assert.ok(result[0].hooks[0].command.includes('custom-lint'), 'user hook preserved'); + }); + + test('entry with only GSD hooks is fully removed', () => { + const entries = [{ + hooks: [ + { type: 'command', command: 'node /path/to/gsd-check-update.js' }, + { type: 'command', command: 'node /path/to/gsd-statusline.js' }, + ], + }]; + + const result = filterGsdHooks(entries); + assert.strictEqual(result.length, 0, 'entry should be removed when all hooks are GSD'); + }); + + test('entry with only user hooks is untouched', () => { + const entries = [{ + matcher: 'Bash', + hooks: [ + { type: 'command', command: 'bash /my/pre-check.sh' }, + ], + }]; + + const result = filterGsdHooks(entries); + assert.strictEqual(result.length, 1, 'entry should survive'); + assert.strictEqual(result[0].hooks.length, 1, 'user hook should remain'); + }); + + test('all GSD hook names are recognized by isGsdHookCommand', () => { + const gsdCommands = [ + 'node /path/gsd-check-update.js', + 'node /path/gsd-statusline.js', + 'bash /path/gsd-session-state.sh', + 'node /path/gsd-context-monitor.js', + 'bash /path/gsd-phase-boundary.sh', + 'node /path/gsd-prompt-guard.js', + 'node /path/gsd-read-guard.js', + 'bash /path/gsd-validate-commit.sh', + 'node /path/gsd-workflow-guard.js', + ]; + + for (const cmd of gsdCommands) { + assert.ok(isGsdHookCommand(cmd), `should recognize: ${cmd}`); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 5. Codex legacy migration +// ───────────────────────────────────────────────────────────────────────────── + +describe('Codex legacy gsd-update-check migration', () => { + test('install.js strips legacy gsd-update-check hook blocks from config', () => { + const src = fs.readFileSync(INSTALL_SRC, 'utf-8'); + assert.ok( + src.includes('gsd-update-check') && src.includes('replace('), + 'install.js should have migration logic to strip legacy gsd-update-check entries' + ); + }); + + test('migration regex removes LF legacy hook block', () => { + const legacyBlock = [ + '[features]', + 'codex_hooks = true', + '', + '# GSD Hooks', + '[[hooks]]', + 'event = "SessionStart"', + 'command = "node /old/path/gsd-update-check.js"', + '', + ].join('\n'); + + let content = legacyBlock; + content = content.replace(/\n# GSD Hooks\n\[\[hooks\]\]\nevent = "SessionStart"\ncommand = "node [^\n]*gsd-update-check\.js"\n/g, '\n'); + assert.ok(!content.includes('gsd-update-check'), 'legacy hook block should be removed'); + assert.ok(content.includes('[features]'), 'non-hook content should be preserved'); + }); + + test('migration regex removes CRLF legacy hook block', () => { + const legacyBlock = [ + '[features]', + 'codex_hooks = true', + '', + '# GSD Hooks', + '[[hooks]]', + 'event = "SessionStart"', + 'command = "node /old/path/gsd-update-check.js"', + '', + ].join('\r\n'); + + let content = legacyBlock; + content = content.replace(/\r\n# GSD Hooks\r\n\[\[hooks\]\]\r\nevent = "SessionStart"\r\ncommand = "node [^\r\n]*gsd-update-check\.js"\r\n/g, '\r\n'); + assert.ok(!content.includes('gsd-update-check'), 'legacy CRLF hook block should be removed'); + assert.ok(content.includes('[features]'), 'non-hook content should be preserved'); + }); +});