diff --git a/.changeset/fix-3357-codex-legacy-hooks-json.md b/.changeset/fix-3357-codex-legacy-hooks-json.md new file mode 100644 index 000000000..59cec1f97 --- /dev/null +++ b/.changeset/fix-3357-codex-legacy-hooks-json.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3364 +--- +**Codex installs now clean up legacy GSD-managed `hooks.json` update hooks after writing the TOML SessionStart hook** — reinstalling no longer leaves duplicate GSD update hooks across `hooks.json` and `config.toml`, while user-owned JSON hooks are preserved. (#3357) diff --git a/bin/install.js b/bin/install.js index 9e349fc6f..a8d25524c 100755 --- a/bin/install.js +++ b/bin/install.js @@ -759,6 +759,76 @@ function rewriteLegacyCodexHookBlock(content, absoluteRunner) { return { content: updated, changed }; } +function isManagedCodexHookCommand(command, targetDir) { + if (typeof command !== 'string') return false; + if (typeof targetDir !== 'string' || targetDir.length === 0) return false; + const normalizedCommand = command.replace(/\\/g, '/'); + const managedHooksDir = `${path.join(targetDir, 'hooks').replace(/\\/g, '/')}/`; + if (!normalizedCommand.includes(managedHooksDir)) return false; + return /(^|[\\/\s"'])(gsd-check-update\.js|gsd-update-check\.js)(?=$|[\s"'])/.test(normalizedCommand); +} + +function pruneGsdManagedHooksJsonValue(value, targetDir) { + if (Array.isArray(value)) { + let changed = false; + const next = []; + for (const item of value) { + const pruned = pruneGsdManagedHooksJsonValue(item, targetDir); + if (pruned.changed) changed = true; + if (!isStructurallyEmpty(pruned.value)) next.push(pruned.value); + else changed = true; + } + return { value: next, changed }; + } + + if (value && typeof value === 'object') { + if (isManagedCodexHookCommand(value.command, targetDir)) { + return { value: null, changed: true }; + } + + let changed = false; + const next = {}; + for (const [key, child] of Object.entries(value)) { + const pruned = pruneGsdManagedHooksJsonValue(child, targetDir); + if (pruned.changed) changed = true; + if (!isStructurallyEmpty(pruned.value)) next[key] = pruned.value; + else changed = true; + } + return { value: next, changed }; + } + + return { value, changed: false }; +} + +function isStructurallyEmpty(value) { + if (value === null || value === undefined) return true; + if (Array.isArray(value)) return value.length === 0; + return typeof value === 'object' && Object.keys(value).length === 0; +} + +function cleanupLegacyCodexHooksJson(targetDir) { + const hooksPath = path.join(targetDir, 'hooks.json'); + if (!fs.existsSync(hooksPath)) return { changed: false, removedFile: false }; + + let parsed; + try { + parsed = JSON.parse(fs.readFileSync(hooksPath, 'utf8')); + } catch { + return { changed: false, removedFile: false, skipped: 'invalid_json' }; + } + + const pruned = pruneGsdManagedHooksJsonValue(parsed, targetDir); + if (!pruned.changed) return { changed: false, removedFile: false }; + + if (isStructurallyEmpty(pruned.value)) { + fs.unlinkSync(hooksPath); + return { changed: true, removedFile: true }; + } + + atomicWriteFileSync(hooksPath, JSON.stringify(pruned.value, null, 2) + '\n', 'utf8'); + return { changed: true, removedFile: false }; +} + /** * Build a hook command path using forward slashes for cross-platform compatibility. * On Windows, $HOME is not expanded by cmd.exe/PowerShell, so we use the actual path. @@ -8586,6 +8656,10 @@ function install(isGlobal, runtime = 'claude') { throw wrapped; } console.log(` ${green}✓${reset} Configured Codex hooks (SessionStart)`); + const legacyHooksCleanup = cleanupLegacyCodexHooksJson(targetDir); + if (legacyHooksCleanup.changed) { + console.log(` ${green}✓${reset} Removed legacy GSD hooks.json entries`); + } } catch (e) { // #2760 — schema-validation and write failures must be loud and fatal // so the user is never left with a config Codex refuses to load (or no @@ -10613,6 +10687,7 @@ if (process.env.GSD_TEST_MODE) { rewriteLegacyManagedNodeHookCommands, buildCodexHookBlock, rewriteLegacyCodexHookBlock, + cleanupLegacyCodexHooksJson, }; } else { diff --git a/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs b/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs new file mode 100644 index 000000000..7bb0a3768 --- /dev/null +++ b/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs @@ -0,0 +1,107 @@ +/** + * Regression test for bug #3357. + * + * Older Codex installs used hooks.json for SessionStart hooks. Current Codex + * installs write config.toml hooks. Reinstalling must remove only GSD-managed + * legacy hooks.json entries so users do not end up with duplicate GSD hooks. + */ + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { install, parseTomlToObject } = require('../bin/install.js'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +function withCodexHome(codexHome, fn) { + const previousCodexHome = process.env.CODEX_HOME; + process.env.CODEX_HOME = codexHome; + try { + return fn(); + } finally { + if (previousCodexHome == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + } +} + +function legacyGsdHook(codexHome) { + return { + hooks: [{ + type: 'command', + command: `node "${path.join(codexHome, 'hooks', 'gsd-check-update.js')}"`, + }], + }; +} + +function userHook() { + return { + hooks: [{ + type: 'command', + command: 'node "/Users/example/bin/user-hook.js"', + }], + }; +} + +function tomlGsdHookCount(codexHome) { + const parsed = parseTomlToObject(fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8')); + const sessionStart = parsed.hooks?.SessionStart ?? []; + return sessionStart + .flatMap((entry) => Array.isArray(entry.hooks) ? entry.hooks : []) + .filter((hook) => typeof hook.command === 'string' && hook.command.includes('gsd-check-update.js')) + .length; +} + +describe('#3357 — Codex install removes legacy GSD hooks.json entries', { concurrency: false }, () => { + let tmpRoot; + let codexHome; + + beforeEach(() => { + tmpRoot = createTempDir('gsd-3357-'); + codexHome = path.join(tmpRoot, '.codex'); + fs.mkdirSync(codexHome, { recursive: true }); + }); + + afterEach(() => { + cleanup(tmpRoot); + }); + + test('removes hooks.json when it only contained the legacy GSD SessionStart hook', () => { + fs.writeFileSync( + path.join(codexHome, 'hooks.json'), + JSON.stringify({ SessionStart: [legacyGsdHook(codexHome)] }, null, 2), + ); + + withCodexHome(codexHome, () => install(true, 'codex')); + + assert.equal(fs.existsSync(path.join(codexHome, 'hooks.json')), false); + assert.equal(tomlGsdHookCount(codexHome), 1); + }); + + test('preserves user hooks.json entries while removing the legacy GSD hook', () => { + const userOwnedSameBasenameHook = { + hooks: [{ + type: 'command', + command: 'node "/Users/example/bin/gsd-check-update.js"', + }], + }; + fs.writeFileSync( + path.join(codexHome, 'hooks.json'), + JSON.stringify({ SessionStart: [legacyGsdHook(codexHome), userHook(), userOwnedSameBasenameHook] }, null, 2), + ); + + withCodexHome(codexHome, () => install(true, 'codex')); + + const hooksJson = JSON.parse(fs.readFileSync(path.join(codexHome, 'hooks.json'), 'utf8')); + const commands = hooksJson.SessionStart.flatMap((entry) => entry.hooks).map((hook) => hook.command); + assert.deepEqual(commands, [ + 'node "/Users/example/bin/user-hook.js"', + 'node "/Users/example/bin/gsd-check-update.js"', + ]); + assert.equal(tomlGsdHookCount(codexHome), 1); + }); +});