From 925cce684a622d93b1f39f695656f617136900c1 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:30:59 -0400 Subject: [PATCH] fix(codex): remove managed hooks.json SessionStart on uninstall --- bin/install.js | 116 +++++++++++------- ...bug-3427-3433-codex-install-shape.test.cjs | 29 ++++- 2 files changed, 97 insertions(+), 48 deletions(-) diff --git a/bin/install.js b/bin/install.js index f3dd63a04..3b29e3059 100755 --- a/bin/install.js +++ b/bin/install.js @@ -843,32 +843,14 @@ function rewriteLegacyCodexHookBlock(content, absoluteRunner, opts) { return { content: updated, changed }; } -/** - * Ensure Codex hooks.json contains exactly one managed SessionStart - * gsd-check-update hook entry, while preserving user-owned entries. - * - * Codex accepts hook config from hooks.json and config.toml. To avoid the - * startup warning for mixed representations in the same layer, GSD now stores - * the managed SessionStart hook in hooks.json and keeps config.toml for - * feature flags / agent metadata only. - * - * Supports both known hooks.json shapes: - * 1) { "SessionStart": [...] } - * 2) { "hooks": { "SessionStart": [...] } } - * - * @param {string} targetDir - * @param {{ absoluteRunner: string|null, platform?: NodeJS.Platform }} opts - * @returns {{ changed: boolean, wrote: boolean, path: string }} - */ -function ensureCodexHooksJsonSessionStart(targetDir, opts = {}) { +function reconcileCodexHooksJsonSessionStart(targetDir, opts = {}) { const hooksJsonPath = path.join(targetDir, 'hooks.json'); - const platform = opts.platform || process.platform; - const absoluteRunner = opts.absoluteRunner || null; - if (!absoluteRunner) return { changed: false, wrote: false, path: hooksJsonPath }; - + const managedCommand = typeof opts.managedCommand === 'string' ? opts.managedCommand : null; let parsed = {}; + let currentContent = null; if (fs.existsSync(hooksJsonPath)) { const raw = fs.readFileSync(hooksJsonPath, 'utf8'); + currentContent = raw; if (raw.trim()) { try { parsed = JSON.parse(raw); @@ -893,11 +875,11 @@ function ensureCodexHooksJsonSessionStart(targetDir, opts = {}) { sanitizedSessionStart.push(entry); continue; } - const keptHooks = originalHooks.filter((hook) => { - const cmd = hook && typeof hook === 'object' ? hook.command : null; - const managed = isManagedHookCommand(cmd, { - surface: 'codex-hooks-json', - includeLegacyAliases: true, + const keptHooks = originalHooks.filter((hook) => { + const cmd = hook && typeof hook === 'object' ? hook.command : null; + const managed = isManagedHookCommand(cmd, { + surface: 'codex-hooks-json', + includeLegacyAliases: true, configDir: targetDir, }); if (managed) removedLegacy = true; @@ -908,6 +890,56 @@ function ensureCodexHooksJsonSessionStart(targetDir, opts = {}) { sanitizedSessionStart.push(nextEntry); } + if (managedCommand) { + sanitizedSessionStart.push({ + hooks: [ + { + type: 'command', + command: managedCommand, + }, + ], + }); + } + + if (sanitizedSessionStart.length > 0) { + hookTable.SessionStart = sanitizedSessionStart; + } else { + delete hookTable.SessionStart; + } + if (usesNestedHooksObject) parsed.hooks = hookTable; + + const nextContent = `${JSON.stringify(parsed, null, 2)}\n`; + const changed = currentContent !== nextContent; + const shouldWrite = changed && (currentContent !== null || Object.keys(parsed).length > 0); + if (shouldWrite) { + atomicWriteFileSync(hooksJsonPath, nextContent, 'utf8'); + } + + return { changed: changed || removedLegacy, wrote: shouldWrite, path: hooksJsonPath }; +} + +/** + * Ensure Codex hooks.json contains exactly one managed SessionStart + * gsd-check-update hook entry, while preserving user-owned entries. + * + * Codex accepts hook config from hooks.json and config.toml. To avoid the + * startup warning for mixed representations in the same layer, GSD now stores + * the managed SessionStart hook in hooks.json and keeps config.toml for + * feature flags / agent metadata only. + * + * Supports both known hooks.json shapes: + * 1) { "SessionStart": [...] } + * 2) { "hooks": { "SessionStart": [...] } } + * + * @param {string} targetDir + * @param {{ absoluteRunner: string|null, platform?: NodeJS.Platform }} opts + * @returns {{ changed: boolean, wrote: boolean, path: string }} + */ +function ensureCodexHooksJsonSessionStart(targetDir, opts = {}) { + const platform = opts.platform || process.platform; + const absoluteRunner = opts.absoluteRunner || null; + const hooksJsonPath = path.join(targetDir, 'hooks.json'); + if (!absoluteRunner) return { changed: false, wrote: false, path: hooksJsonPath }; const managedCommand = projectManagedHookCommand({ absoluteRunner, scriptPath: path.resolve(targetDir, 'hooks', 'gsd-check-update.js'), @@ -915,27 +947,11 @@ function ensureCodexHooksJsonSessionStart(targetDir, opts = {}) { platform, }); if (!managedCommand) return { changed: false, wrote: false, path: hooksJsonPath }; + return reconcileCodexHooksJsonSessionStart(targetDir, { managedCommand }); +} - sanitizedSessionStart.push({ - hooks: [ - { - type: 'command', - command: managedCommand, - }, - ], - }); - - hookTable.SessionStart = sanitizedSessionStart; - if (usesNestedHooksObject) parsed.hooks = hookTable; - - const nextContent = `${JSON.stringify(parsed, null, 2)}\n`; - const currentContent = fs.existsSync(hooksJsonPath) ? fs.readFileSync(hooksJsonPath, 'utf8') : null; - const changed = currentContent !== nextContent; - if (changed) { - atomicWriteFileSync(hooksJsonPath, nextContent, 'utf8'); - } - - return { changed: changed || removedLegacy, wrote: changed, path: hooksJsonPath }; +function removeCodexHooksJsonSessionStart(targetDir) { + return reconcileCodexHooksJsonSessionStart(targetDir, { managedCommand: null }); } /** @@ -6530,6 +6546,12 @@ function uninstall(isGlobal, runtime = 'claude') { console.log(` ${green}✓${reset} Cleaned GSD sections from config.toml`); } } + + const hooksJsonCleanup = removeCodexHooksJsonSessionStart(targetDir); + if (hooksJsonCleanup.changed) { + removedCount++; + console.log(` ${green}✓${reset} Removed managed Codex SessionStart hook from hooks.json`); + } } } else if (isCopilot) { // Copilot: remove skills/gsd-*/ directories (same layout as Codex skills) diff --git a/tests/bug-3427-3433-codex-install-shape.test.cjs b/tests/bug-3427-3433-codex-install-shape.test.cjs index 59cd5c4e8..6d4c5d60e 100644 --- a/tests/bug-3427-3433-codex-install-shape.test.cjs +++ b/tests/bug-3427-3433-codex-install-shape.test.cjs @@ -9,7 +9,7 @@ const path = require('node:path'); const crypto = require('node:crypto'); const { execFileSync } = require('node:child_process'); -const { install, parseTomlToObject } = require('../bin/install.js'); +const { install, uninstall, parseTomlToObject } = require('../bin/install.js'); const { createTempDir, cleanup } = require('./helpers.cjs'); const HOOKS_DIST = path.join(__dirname, '..', 'hooks', 'dist'); @@ -120,4 +120,31 @@ describe('#3427 + #3433 — Codex installer avoids duplicate skills and mixed ho assert.equal(gsdCommands.length, 1); assert.equal(sessionStartCommands.includes('node "/Users/example/bin/user-hook.js"'), true); }); + + test('uninstall removes managed SessionStart hook from hooks.json but preserves user hooks', () => { + const hooksDir = path.join(codexHome, 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync(path.join(hooksDir, 'gsd-check-update.js'), '// managed hook\n'); + const managedHookPath = path.join(codexHome, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); + + fs.writeFileSync(path.join(codexHome, 'hooks.json'), JSON.stringify({ + SessionStart: [ + { + hooks: [ + { type: 'command', command: `node "${managedHookPath}"` }, + { type: 'command', command: 'node "/Users/example/bin/user-hook.js"' }, + ], + }, + ], + }, null, 2)); + + withCodexHome(codexHome, () => uninstall(true, 'codex')); + + const hooksJson = JSON.parse(fs.readFileSync(path.join(codexHome, 'hooks.json'), 'utf8')); + const sessionStartCommands = extractSessionStartCommandsFromHooksJson(hooksJson); + const gsdCommands = sessionStartCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + + assert.equal(gsdCommands.length, 0); + assert.equal(sessionStartCommands.includes('node "/Users/example/bin/user-hook.js"'), true); + }); });