From 1d6284a718397ede127a8726552c6054dc367ea8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:17:39 -0400 Subject: [PATCH 1/7] fix(codex): remove duplicate skill copies and consolidate hook payloads --- bin/install.js | 198 ++++++++++++++---- .../bug-2760-codex-install-defensive.test.cjs | 118 +++++------ ...codex-legacy-hooks-json-migration.test.cjs | 32 ++- ...bug-3427-3433-codex-install-shape.test.cjs | 123 +++++++++++ 4 files changed, 351 insertions(+), 120 deletions(-) create mode 100644 tests/bug-3427-3433-codex-install-shape.test.cjs diff --git a/bin/install.js b/bin/install.js index 801a1af25..f3dd63a04 100755 --- a/bin/install.js +++ b/bin/install.js @@ -843,6 +843,101 @@ 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 = {}) { + 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 }; + + let parsed = {}; + if (fs.existsSync(hooksJsonPath)) { + const raw = fs.readFileSync(hooksJsonPath, 'utf8'); + if (raw.trim()) { + try { + parsed = JSON.parse(raw); + } catch (err) { + throw new Error(`hooks.json parse failed: ${err && err.message ? err.message : String(err)}`); + } + } + } + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) parsed = {}; + + const usesNestedHooksObject = + parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks); + const hookTable = usesNestedHooksObject ? parsed.hooks : parsed; + const sessionStart = Array.isArray(hookTable.SessionStart) ? hookTable.SessionStart : []; + + let removedLegacy = false; + const sanitizedSessionStart = []; + for (const entry of sessionStart) { + if (!entry || typeof entry !== 'object' || Array.isArray(entry)) continue; + const originalHooks = Array.isArray(entry.hooks) ? entry.hooks : []; + if (originalHooks.length === 0) { + 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, + configDir: targetDir, + }); + if (managed) removedLegacy = true; + return !managed; + }); + if (keptHooks.length === 0) continue; + const nextEntry = { ...entry, hooks: keptHooks }; + sanitizedSessionStart.push(nextEntry); + } + + const managedCommand = projectManagedHookCommand({ + absoluteRunner, + scriptPath: path.resolve(targetDir, 'hooks', 'gsd-check-update.js'), + runtime: 'codex', + platform, + }); + if (!managedCommand) return { changed: false, wrote: false, path: hooksJsonPath }; + + 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 }; +} + /** * 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. @@ -7918,13 +8013,22 @@ function install(isGlobal, runtime = 'claude', options = {}) { } } else if (isCodex) { const skillsDir = path.join(targetDir, 'skills'); - const gsdSrc = _stageSkills(_commandsDir); - copyCommandsAsCodexSkills(gsdSrc, skillsDir, 'gsd', pathPrefix, runtime); - const installedSkillNames = listCodexSkillNames(skillsDir); - if (installedSkillNames.length > 0) { - console.log(` ${green}✓${reset} Installed ${installedSkillNames.length} skills to skills/`); + // Codex now discovers repo/user/admin/system skills from .agents/skills and + // warns if a layer mixes redundant hook/skill representations. Legacy + // gsd-* copies under ~/.codex/skills are therefore removed and no longer + // regenerated. + let removedLegacyCodexSkills = 0; + if (fs.existsSync(skillsDir)) { + for (const entry of fs.readdirSync(skillsDir, { withFileTypes: true })) { + if (!entry.isDirectory() || !entry.name.startsWith('gsd-')) continue; + fs.rmSync(path.join(skillsDir, entry.name), { recursive: true, force: true }); + removedLegacyCodexSkills += 1; + } + } + if (removedLegacyCodexSkills > 0) { + console.log(` ${green}✓${reset} Removed ${removedLegacyCodexSkills} legacy Codex gsd-* skill copies from skills/`); } else { - failures.push('skills/gsd-*'); + console.log(` ${dim}↳${reset} Skipped Codex skill-copy generation (Codex discovers official skills directly)`); } } else if (isCopilot) { const skillsDir = path.join(targetDir, 'skills'); @@ -8535,7 +8639,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { } if (isCodex && !isMinimalMode(_effectiveInstallMode)) { - // Capture pre-install snapshot of config.toml before ANY GSD mutation + // Capture pre-install snapshots before ANY GSD mutation // (#2760 fix 3). On post-write schema-validation failure OR any throw // during the mutation sequence (write failure, merge throw, etc.) we // restore these exact bytes so the user is never left with a broken @@ -8546,9 +8650,19 @@ function install(isGlobal, runtime = 'claude', options = {}) { const codexConfigPreInstallSnapshot = fs.existsSync(codexConfigPathPreInstall) ? fs.readFileSync(codexConfigPathPreInstall) : null; + const codexHooksJsonPathPreInstall = path.join(targetDir, 'hooks.json'); + const codexHooksJsonPreInstallSnapshot = fs.existsSync(codexHooksJsonPathPreInstall) + ? fs.readFileSync(codexHooksJsonPathPreInstall) + : null; + const migrationTouchesHooksJson = + !!(installerMigrationResult + && installerMigrationResult.plan + && Array.isArray(installerMigrationResult.plan.actions) + && installerMigrationResult.plan.actions.some((action) => action && action.relPath === 'hooks.json')); // #3245 — unified idempotent rollback. Reverts ALL Codex-specific mutations: // config.toml — restore pre-install bytes (or remove if was absent) + // hooks.json — restore pre-install bytes (or remove if was absent) // skills/gsd-* — restore pre-existing dirs from content snapshot; remove // newly-created dirs (i.e. those not in the pre-install Set) // agents/gsd-* — restore pre-existing files from content snapshot; remove @@ -8569,6 +8683,19 @@ function install(isGlobal, runtime = 'claude', options = {}) { try { fs.rmSync(codexConfigPathPreInstall); } catch (_) { /* best-effort */ } } + // 1b. hooks.json + // If installer migrations touched hooks.json, rollbackInstallerMigrations() + // already restored the pre-migration file. Don't overwrite that state with + // a post-migration snapshot. + if (!migrationTouchesHooksJson) { + if (codexHooksJsonPreInstallSnapshot !== null) { + try { fs.writeFileSync(codexHooksJsonPathPreInstall, codexHooksJsonPreInstallSnapshot); } + catch (_) { /* best-effort restore — surface the original error */ } + } else if (fs.existsSync(codexHooksJsonPathPreInstall)) { + try { fs.rmSync(codexHooksJsonPathPreInstall); } catch (_) { /* best-effort */ } + } + } + // 2. skills/gsd-* // • Dirs that pre-existed: wipe current contents, restore snapshotted files. // The restore iterates the SNAPSHOT manifest (codexPreInstallSkillNames) rather @@ -8688,9 +8815,9 @@ function install(isGlobal, runtime = 'claude', options = {}) { console.log(` ${dim}↳${reset} Skipping Codex agent config generation (minimal install)`); } - // Copy hook files that are referenced in config.toml (#2153) + // Copy hook files that are referenced by Codex hook configuration (#2153) // The main hook-copy block is gated to non-Codex runtimes, but Codex registers - // gsd-check-update.js in config.toml — the file must physically exist. + // gsd-check-update.js through hooks config — the file must physically exist. const codexHooksSrc = path.join(src, 'hooks', 'dist'); if (fs.existsSync(codexHooksSrc)) { const codexHooksDest = path.join(targetDir, 'hooks'); @@ -8758,37 +8885,11 @@ function install(isGlobal, runtime = 'claude', options = {}) { const codexHooksFeature = ensureCodexHooksFeature(configContent); configContent = setManagedCodexHooksOwnership(codexHooksFeature.content, codexHooksFeature.ownership); - // Add SessionStart hook for update checking. Codex 0.124.0+ requires the - // two-level nested AoT schema: [[hooks.SessionStart]] for the event entry - // (holds optional matcher) and [[hooks.SessionStart.hooks]] for the handler - // (holds type, command, statusMessage, timeout). (#2637, #2760, #2773) - // - // #3017: route through buildCodexHookBlock() so the absolute Node binary - // path is emitted (matching the settings.json branch via #3002), so the - // hook resolves under GUI/minimal-PATH runtimes where bare `node` doesn't. + // GSD-managed Codex hook payloads now live in hooks.json to avoid mixed + // representation warnings when a single layer contains both hooks.json + // and inline [hooks] entries. Keep config.toml focused on feature flags + // and agent metadata. const codexNodeRunner = resolveNodeRunner(); - const hookBlock = buildCodexHookBlock(targetDir, { absoluteRunner: codexNodeRunner, eol }); - - if (hasEnabledCodexHooksFeature(configContent)) { - // Reinstall path: rewrite a legacy bare-node managed-hook entry to the - // absolute runner. Mirrors rewriteLegacyManagedNodeHookCommands for the - // settings.json surface (#3002 CR). - const rewrite = rewriteLegacyCodexHookBlock(configContent, codexNodeRunner); - if (rewrite.changed) { - configContent = rewrite.content; - console.log(` ${green}✓${reset} Migrated legacy bare-node Codex hook to absolute runner (#3017)`); - } - if (!configContent.includes('gsd-check-update')) { - if (hookBlock !== null) { - configContent += hookBlock; - } else { - // resolveNodeRunner() returned null — process.execPath unavailable. - // Match the settings.json branch's warn-and-skip behavior rather - // than emit a broken bare-node hook (the #2979 / #3017 failure mode). - console.warn(` ${yellow}⚠${reset} Skipping Codex SessionStart hook registration — Node executable path unavailable (process.execPath is empty). See #2979 / #3002 / #3017.`); - } - } - } // #2760 fix 3 — post-write schema validation. Parse the bytes we are // about to commit and assert they match Codex's expected shape. If @@ -8826,7 +8927,24 @@ function install(isGlobal, runtime = 'claude', options = {}) { ); throw wrapped; } - console.log(` ${green}✓${reset} Configured Codex hooks (SessionStart)`); + if (hasEnabledCodexHooksFeature(configContent)) { + const checkUpdateFile = path.join(targetDir, 'hooks', 'gsd-check-update.js'); + if (!fs.existsSync(checkUpdateFile)) { + console.warn(` ${yellow}⚠${reset} Skipped Codex SessionStart hook registration — gsd-check-update.js not found at target`); + } else if (!codexNodeRunner) { + console.warn(` ${yellow}⚠${reset} Skipping Codex SessionStart hook registration — Node executable path unavailable (process.execPath is empty). See #2979 / #3002 / #3017.`); + } else { + const hookWrite = ensureCodexHooksJsonSessionStart(targetDir, { + absoluteRunner: codexNodeRunner, + platform: process.platform, + }); + if (hookWrite.wrote) { + console.log(` ${green}✓${reset} Configured Codex hooks (SessionStart via hooks.json)`); + } else { + console.log(` ${green}✓${reset} Verified Codex hooks (SessionStart via hooks.json)`); + } + } + } } 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 diff --git a/tests/bug-2760-codex-install-defensive.test.cjs b/tests/bug-2760-codex-install-defensive.test.cjs index dca40cbaa..e6fc68a8d 100644 --- a/tests/bug-2760-codex-install-defensive.test.cjs +++ b/tests/bug-2760-codex-install-defensive.test.cjs @@ -77,6 +77,27 @@ function writeCodexConfig(codexHome, content) { fs.writeFileSync(path.join(codexHome, 'config.toml'), content, 'utf8'); } +function readCodexHooksJson(codexHome) { + const hooksPath = path.join(codexHome, 'hooks.json'); + if (!fs.existsSync(hooksPath)) return {}; + const raw = fs.readFileSync(hooksPath, 'utf8').trim(); + if (!raw) return {}; + return JSON.parse(raw); +} + +function readHooksSessionStartCommands(codexHome) { + const parsed = readCodexHooksJson(codexHome); + const table = (parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks)) + ? parsed.hooks + : parsed; + const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : []; + return sessionStart.flatMap((entry) => + (Array.isArray(entry?.hooks) ? entry.hooks : []) + .map((hook) => hook && hook.command) + .filter((cmd) => typeof cmd === 'string') + ); +} + describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/reinstall', () => { let tmpDir; let codexHome; @@ -99,37 +120,16 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei const content = readCodexConfig(codexHome); const parsed = parseTomlToObject(content); - // hooks must be an object (namespaced), NOT a flat array. + const sessionStartCommands = readHooksSessionStartCommands(codexHome); + const managed = sessionStartCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd)); + assert.equal(managed.length, 1, 'hooks.json must contain exactly one managed gsd-check-update command'); assert.ok( - parsed.hooks && !Array.isArray(parsed.hooks) && typeof parsed.hooks === 'object', - 'hooks must be a namespaced object, not a flat array: got ' + JSON.stringify(parsed.hooks) - ); - // hooks.SessionStart must be an array-of-tables. - assert.ok( - Array.isArray(parsed.hooks.SessionStart), - 'hooks.SessionStart must be array-of-tables: got ' + typeof parsed.hooks.SessionStart - ); - // Each event entry must have a .hooks sub-array. - const eventEntry = parsed.hooks.SessionStart[0]; - assert.ok( - eventEntry && Array.isArray(eventEntry.hooks), - 'hooks.SessionStart[0].hooks must be an array of handlers: got ' + JSON.stringify(eventEntry) - ); - // The handler must have type = "command" and reference gsd-check-update.js. - const handler = eventEntry.hooks[0]; - assert.strictEqual(handler.type, 'command', 'handler type must be "command"'); - assert.ok( - typeof handler.command === 'string' && /gsd-check-update\.js/.test(handler.command), - 'handler command must reference gsd-check-update.js: got ' + handler.command - ); - // No flat [[hooks]] entries must exist alongside the namespaced form. - assert.ok( - !Array.isArray(parsed.hooks), - 'flat [[hooks]] AoT must not coexist with namespaced [[hooks.SessionStart]]' + !parsed.hooks || !Array.isArray(parsed.hooks.SessionStart), + 'config.toml should not carry managed SessionStart hooks for GSD' ); }); - test('preserves user [[hooks.SessionStart]] entries and adds GSD nested handler', () => { + test('preserves user [[hooks.SessionStart]] entries and registers managed GSD handler in hooks.json', () => { // Users may have their own [[hooks.SessionStart]] entries using the new schema. // GSD must append its own two-level block without disturbing theirs. const userConfig = [ @@ -170,9 +170,10 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei allCommands.includes('echo second user hook'), 'second user hook preserved: ' + JSON.stringify(allCommands) ); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); assert.ok( - allCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), - 'GSD handler must appear in hooks.SessionStart[].hooks: ' + JSON.stringify(allCommands) + hooksJsonCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), + 'GSD handler must appear in hooks.json SessionStart entries: ' + JSON.stringify(hooksJsonCommands) ); assert.ok(!Array.isArray(parsed.hooks), 'no flat [[hooks]] entries'); }); @@ -197,16 +198,9 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei // Old flat form must be gone. assert.ok(!Array.isArray(parsed.hooks), 'flat [[hooks]] must be stripped on upgrade'); - // New nested form must be present. - assert.ok(Array.isArray(parsed.hooks && parsed.hooks.SessionStart), 'new [[hooks.SessionStart]] must be present'); - const handler = parsed.hooks.SessionStart[0].hooks[0]; - assert.strictEqual(handler.type, 'command'); - assert.ok(/gsd-check-update\.js/.test(handler.command)); - // Only one GSD hook entry must exist (no duplication). - const sessionStart = parsed.hooks?.SessionStart ?? []; - const gsdHandlers = sessionStart.flatMap((entry) => - Array.isArray(entry.hooks) ? entry.hooks : [] - ).filter((h) => typeof h?.command === 'string' && /gsd-check-update\.js/.test(h.command)); + // Only one GSD hook entry must exist (no duplication) in hooks.json. + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdHandlers = hooksJsonCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd)); assert.strictEqual(gsdHandlers.length, 1, 'exactly one managed handler after upgrade'); }); @@ -228,20 +222,9 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei const content = readCodexConfig(codexHome); const parsed = parseTomlToObject(content); - assert.ok(Array.isArray(parsed.hooks && parsed.hooks.SessionStart), '[[hooks.SessionStart]] must be present'); - const eventEntry = parsed.hooks.SessionStart[0]; - assert.ok(Array.isArray(eventEntry.hooks), '[[hooks.SessionStart.hooks]] sub-table must be present'); - const handler = eventEntry.hooks[0]; - assert.strictEqual(handler.type, 'command'); - assert.ok(/gsd-check-update\.js/.test(handler.command)); - const handlers = (parsed.hooks?.SessionStart ?? []).flatMap((entry) => - Array.isArray(entry.hooks) ? entry.hooks : [] - ); - assert.strictEqual( - handlers.filter((h) => typeof h?.command === 'string' && /gsd-check-update\.js/.test(h.command)).length, - 1, - 'exactly one managed handler after upgrade from PR-#2802-shape' - ); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdHandlers = hooksJsonCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd)); + assert.strictEqual(gsdHandlers.length, 1, 'exactly one managed handler after upgrade from PR-#2802-shape'); }); test('reinstall is idempotent: correct nested schema is stripped and re-emitted cleanly', () => { @@ -250,12 +233,9 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei runCodexInstall(codexHome); // second install const content = readCodexConfig(codexHome); - const parsed = parseTomlToObject(content); - const sessionStart = parsed.hooks?.SessionStart ?? []; - assert.strictEqual(sessionStart.length, 1, 'exactly one SessionStart event entry after double install'); - assert.ok(Array.isArray(sessionStart[0].hooks), 'SessionStart event has nested handlers array'); - assert.strictEqual(sessionStart[0].hooks.length, 1, 'exactly one handler in SessionStart after double install'); - assert.ok(/gsd-check-update\.js/.test(sessionStart[0].hooks[0].command), 'managed handler command preserved'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdHandlers = hooksJsonCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd)); + assert.strictEqual(gsdHandlers.length, 1, 'exactly one managed SessionStart handler after double install'); }); }); @@ -686,10 +666,11 @@ describe('#2760 CR4 finding 2 — Legacy flat [[hooks]] block migrates to namesp allSessionStartCommands.includes('echo user hook'), 'user [[hooks.SessionStart]] entry preserved: ' + JSON.stringify(allSessionStartCommands) ); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); assert.ok( - allSessionStartCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), - 'GSD entry must appear in hooks.SessionStart array (namespaced AoT form): ' - + JSON.stringify(allSessionStartCommands) + hooksJsonCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), + 'GSD entry must appear in hooks.json SessionStart entries: ' + + JSON.stringify(hooksJsonCommands) ); // The legacy top-level [[hooks]] AoT must NOT coexist with the namespaced @@ -701,9 +682,7 @@ describe('#2760 CR4 finding 2 — Legacy flat [[hooks]] block migrates to namesp ); // No duplicate gsd-check-update entries — exactly one managed entry. - const gsdEntries = allSessionStartCommands.filter( - (cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd) - ); + const gsdEntries = hooksJsonCommands.filter((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)); assert.equal(gsdEntries.length, 1, 'exactly one gsd-check-update entry after migration, got: ' + gsdEntries.length); }); @@ -809,7 +788,7 @@ describe('#2760 CR4 finding 1 — atomicWriteFileSync failure aborts install (po let isHookWrite = false; try { const data = fs.readFileSync(src, 'utf8'); - isHookWrite = /gsd-check-update\.js/.test(data); + isHookWrite = /GSD codex_hooks ownership/.test(data); } catch (_) { /* ignore */ } if (isHookWrite) { throw new Error('simulated rename failure'); @@ -1083,10 +1062,11 @@ describe('#2760 CR5 finding 3 — migration emits namespaced AoT (no flat/namesp JSON.stringify(ssCommands) ); // GSD's managed gsd-check-update entry also lives in the namespaced array. + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); assert.ok( - ssCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), - 'managed gsd-check-update entry must appear in hooks.SessionStart array: ' + - JSON.stringify(ssCommands) + hooksJsonCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), + 'managed gsd-check-update entry must appear in hooks.json SessionStart entries: ' + + JSON.stringify(hooksJsonCommands) ); // No flat top-level [[hooks]] AoT may remain. diff --git a/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs b/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs index b88c15831..9b0ce6bbd 100644 --- a/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs +++ b/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs @@ -1,9 +1,10 @@ /** * 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. + * Older Codex installs carried legacy GSD SessionStart commands in hooks.json. + * Current install keeps the managed SessionStart hook in hooks.json (single + * representation per layer) and strips stale managed entries before writing + * exactly one canonical managed command. */ 'use strict'; @@ -14,11 +15,14 @@ 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 { execFileSync } = require('node:child_process'); const installModule = require('../bin/install.js'); const { readInstallState } = require('../get-shit-done/bin/lib/installer-migrations.cjs'); const { install, parseTomlToObject } = installModule; const { createTempDir, cleanup } = require('./helpers.cjs'); +const HOOKS_DIST = path.join(__dirname, '..', 'hooks', 'dist'); +const BUILD_HOOKS_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); function withCodexHome(codexHome, fn) { const previousCodexHome = process.env.CODEX_HOME; @@ -63,6 +67,9 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc let codexHome; beforeEach(() => { + if (!fs.existsSync(HOOKS_DIST) || fs.readdirSync(HOOKS_DIST).length === 0) { + execFileSync(process.execPath, [BUILD_HOOKS_SCRIPT], { stdio: 'pipe' }); + } tmpRoot = createTempDir('gsd-3357-'); codexHome = path.join(tmpRoot, '.codex'); fs.mkdirSync(codexHome, { recursive: true }); @@ -73,7 +80,7 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc cleanup(tmpRoot); }); - test('removes hooks.json when it only contained the legacy GSD SessionStart hook', () => { + test('rewrites hooks.json to one managed SessionStart hook when file only had legacy managed entry', () => { fs.writeFileSync( path.join(codexHome, 'hooks.json'), JSON.stringify({ SessionStart: [legacyGsdHook(codexHome)] }, null, 2), @@ -81,8 +88,11 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc withCodexHome(codexHome, () => install(true, 'codex')); - assert.equal(fs.existsSync(path.join(codexHome, 'hooks.json')), false); - assert.equal(tomlGsdHookCount(codexHome), 1); + const hooksJson = JSON.parse(fs.readFileSync(path.join(codexHome, 'hooks.json'), 'utf8')); + const commands = hooksJson.SessionStart.flatMap((entry) => entry.hooks).map((hook) => hook.command); + const managed = commands.filter((cmd) => typeof cmd === 'string' && cmd.includes('gsd-check-update.js')); + assert.equal(managed.length, 1); + assert.equal(tomlGsdHookCount(codexHome), 0); }); test('preserves user hooks.json entries while removing the legacy GSD hook', () => { @@ -101,11 +111,11 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc 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); + const managed = commands.filter((cmd) => typeof cmd === 'string' && cmd.includes('gsd-check-update.js')); + assert.equal(commands.includes('node "/Users/example/bin/user-hook.js"'), true); + assert.equal(commands.includes('node "/Users/example/bin/gsd-check-update.js"'), true); + assert.equal(managed.length, 2); + assert.equal(tomlGsdHookCount(codexHome), 0); }); test('restores migrated hooks.json and install state when later Codex validation fails', () => { diff --git a/tests/bug-3427-3433-codex-install-shape.test.cjs b/tests/bug-3427-3433-codex-install-shape.test.cjs new file mode 100644 index 000000000..59cd5c4e8 --- /dev/null +++ b/tests/bug-3427-3433-codex-install-shape.test.cjs @@ -0,0 +1,123 @@ +'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 crypto = require('node:crypto'); +const { execFileSync } = require('node:child_process'); + +const { install, parseTomlToObject } = require('../bin/install.js'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const HOOKS_DIST = path.join(__dirname, '..', 'hooks', 'dist'); +const BUILD_HOOKS_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); + +function withCodexHome(codexHome, fn) { + const prev = process.env.CODEX_HOME; + process.env.CODEX_HOME = codexHome; + try { + return fn(); + } finally { + if (prev == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = prev; + } +} + +function extractSessionStartCommandsFromHooksJson(value) { + if (!value || typeof value !== 'object' || Array.isArray(value)) return []; + const table = (value.hooks && typeof value.hooks === 'object' && !Array.isArray(value.hooks)) + ? value.hooks + : value; + const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : []; + return sessionStart.flatMap((entry) => { + const hooks = entry && Array.isArray(entry.hooks) ? entry.hooks : []; + return hooks.map((h) => h && h.command).filter((cmd) => typeof cmd === 'string'); + }); +} + +describe('#3427 + #3433 — Codex installer avoids duplicate skills and mixed hook representation', { concurrency: false }, () => { + let tmpRoot; + let codexHome; + + beforeEach(() => { + if (!fs.existsSync(HOOKS_DIST) || fs.readdirSync(HOOKS_DIST).length === 0) { + execFileSync(process.execPath, [BUILD_HOOKS_SCRIPT], { stdio: 'pipe' }); + } + tmpRoot = createTempDir('gsd-3427-3433-'); + codexHome = path.join(tmpRoot, '.codex'); + fs.mkdirSync(codexHome, { recursive: true }); + }); + + afterEach(() => { + cleanup(tmpRoot); + }); + + test('removes legacy gsd-* copies from ~/.codex/skills and does not regenerate them', () => { + const legacySkillBody = '# old managed\n'; + fs.mkdirSync(path.join(codexHome, 'skills', 'gsd-help'), { recursive: true }); + fs.writeFileSync(path.join(codexHome, 'skills', 'gsd-help', 'SKILL.md'), legacySkillBody); + const legacyHash = crypto.createHash('sha256').update(legacySkillBody).digest('hex'); + fs.writeFileSync(path.join(codexHome, 'gsd-file-manifest.json'), JSON.stringify({ + version: 1, + files: { + 'skills/gsd-help/SKILL.md': legacyHash, + }, + }, null, 2)); + + fs.mkdirSync(path.join(codexHome, 'skills', 'custom-user-skill'), { recursive: true }); + fs.writeFileSync(path.join(codexHome, 'skills', 'custom-user-skill', 'SKILL.md'), '# user skill\n'); + + withCodexHome(codexHome, () => install(true, 'codex')); + + const skillsDir = path.join(codexHome, 'skills'); + const entries = fs.existsSync(skillsDir) + ? fs.readdirSync(skillsDir, { withFileTypes: true }).filter((e) => e.isDirectory()).map((e) => e.name) + : []; + + assert.equal(entries.some((name) => name.startsWith('gsd-')), false); + assert.equal(entries.includes('custom-user-skill'), true); + }); + + test('stores managed SessionStart update hook in hooks.json and removes inline gsd hook from config.toml', () => { + const configToml = [ + '[features]', + 'codex_hooks = true', + '', + '[[hooks.SessionStart]]', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', + 'command = "node /tmp/legacy/.codex/hooks/gsd-check-update.js"', + '', + ].join('\n'); + fs.writeFileSync(path.join(codexHome, 'config.toml'), configToml); + + fs.writeFileSync(path.join(codexHome, 'hooks.json'), JSON.stringify({ + SessionStart: [ + { + hooks: [ + { type: 'command', command: 'node "/Users/example/bin/user-hook.js"' }, + ], + }, + ], + }, null, 2)); + + withCodexHome(codexHome, () => install(true, 'codex')); + + const parsedToml = parseTomlToObject(fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8')); + const tomlSessionStart = parsedToml.hooks?.SessionStart ?? []; + const tomlCommands = tomlSessionStart.flatMap((entry) => + (Array.isArray(entry?.hooks) ? entry.hooks : []).map((hook) => hook.command).filter((cmd) => typeof cmd === 'string') + ); + assert.equal(tomlCommands.some((cmd) => cmd.includes('gsd-check-update.js')), false); + + 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, 1); + assert.equal(sessionStartCommands.includes('node "/Users/example/bin/user-hook.js"'), true); + }); +}); From 51f549caaf5742af8d8e085ccb96c9899577caca Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:27:12 -0400 Subject: [PATCH 2/7] test(codex): align install contracts with no gsd skill copies --- tests/install-minimal-all-runtimes.test.cjs | 10 ++++++++-- ...nstaller-migration-install-integration.test.cjs | 14 +++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/install-minimal-all-runtimes.test.cjs b/tests/install-minimal-all-runtimes.test.cjs index 8d109e20b..b3ea99a70 100644 --- a/tests/install-minimal-all-runtimes.test.cjs +++ b/tests/install-minimal-all-runtimes.test.cjs @@ -179,6 +179,12 @@ function expectedSkillSet() { return new Set([...MINIMAL_SKILL_ALLOWLIST]); } +function expectedManifestSkillSet(runtime) { + // Codex no longer materializes gsd-* skill files in minimal mode. + if (runtime === 'codex') return new Set(); + return expectedSkillSet(); +} + describe('install: --minimal honoured for every runtime in --global mode', () => { for (const runtime of SKILL_RUNTIMES) { test(`${runtime} --global --minimal emits exactly the core skill set, zero agents`, () => { @@ -193,7 +199,7 @@ describe('install: --minimal honoured for every runtime in --global mode', () => `${runtime} global manifest.mode should be "minimal"`); assert.deepStrictEqual( [...manifestSkillSet(manifest)].sort(), - [...expectedSkillSet()].sort(), + [...expectedManifestSkillSet(runtime)].sort(), `${runtime} global should record exactly the MINIMAL allowlist in the manifest`, ); assert.strictEqual(manifestAgentCount(manifest), 0, @@ -219,7 +225,7 @@ describe('install: --minimal honoured for every runtime in --local mode', () => `${runtime} local manifest.mode should be "minimal"`); assert.deepStrictEqual( [...manifestSkillSet(manifest)].sort(), - [...expectedSkillSet()].sort(), + [...expectedManifestSkillSet(runtime)].sort(), `${runtime} local should record exactly the MINIMAL allowlist in the manifest (regression guard for #2923)`, ); assert.strictEqual(manifestAgentCount(manifest), 0, diff --git a/tests/installer-migration-install-integration.test.cjs b/tests/installer-migration-install-integration.test.cjs index 1b2565725..f9ca22a72 100644 --- a/tests/installer-migration-install-integration.test.cjs +++ b/tests/installer-migration-install-integration.test.cjs @@ -174,6 +174,14 @@ function assertHasGsdDirectory(root, relPath) { ); } +function assertNoGsdDirectoryEntries(root, relPath) { + assert.equal( + listDirNames(root, relPath).some((name) => name.startsWith('gsd-')), + false, + `${relPath} should not contain generated GSD entries` + ); +} + function assertFreshInstallContract(runtime, targetDir) { const contract = RUNTIME_INSTALL_CONTRACTS[runtime]; assert.ok(contract, `missing runtime install contract for ${runtime}`); @@ -201,7 +209,11 @@ function assertFreshInstallContract(runtime, targetDir) { ); if (contract.surface === 'flat-skills') { - assertHasGsdDirectory(targetDir, 'skills'); + if (runtime === 'codex') { + assertNoGsdDirectoryEntries(targetDir, 'skills'); + } else { + assertHasGsdDirectory(targetDir, 'skills'); + } } else if (contract.surface === 'hermes-skills') { assertHasGsdDirectory(targetDir, path.join('skills', 'gsd')); assert.ok( From 925cce684a622d93b1f39f695656f617136900c1 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:30:59 -0400 Subject: [PATCH 3/7] 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); + }); }); From 9dc9e3744539c688e6d5d760f3617008fdd25492 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:38:41 -0400 Subject: [PATCH 4/7] test(codex): align hook assertions with hooks.json --- tests/codex-config.test.cjs | 99 ++++++++++++++++++++++++++----------- 1 file changed, 69 insertions(+), 30 deletions(-) diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index f27ce2b89..5be0a224f 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -81,6 +81,23 @@ function writeCodexConfig(codexHome, content) { fs.writeFileSync(path.join(codexHome, 'config.toml'), content, 'utf8'); } +function readHooksSessionStartCommands(codexHome) { + const hooksPath = path.join(codexHome, 'hooks.json'); + if (!fs.existsSync(hooksPath)) return []; + const raw = fs.readFileSync(hooksPath, 'utf8').trim(); + if (!raw) return []; + const parsed = JSON.parse(raw); + const table = (parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks)) + ? parsed.hooks + : parsed; + const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : []; + return sessionStart.flatMap((entry) => + (Array.isArray(entry?.hooks) ? entry.hooks : []) + .map((hook) => hook && hook.command) + .filter((cmd) => typeof cmd === 'string') + ); +} + function countMatches(content, pattern) { return (content.match(pattern) || []).length; } @@ -1409,14 +1426,19 @@ describe('Codex install hook configuration (e2e)', () => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); - test('Codex install copies hook file that is referenced in config.toml (#2153)', () => { + test('Codex install copies hook file that is referenced in hooks.json (#2153)', () => { // Regression test: Codex install writes gsd-check-update hook reference into - // config.toml but must also copy the hook file to ~/$CODEX_HOME/hooks/ + // hooks.json and must also copy the hook file to ~/$CODEX_HOME/hooks/ runCodexInstall(codexHome); const configContent = readCodexConfig(codexHome); - // config.toml must reference the hook - assert.ok(configContent.includes('gsd-check-update.js'), 'config.toml references gsd-check-update.js'); + assert.ok(!configContent.includes('gsd-check-update.js'), 'config.toml does not carry managed SessionStart hooks'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + assert.equal( + hooksJsonCommands.some((cmd) => cmd.includes('gsd-check-update.js')), + true, + 'hooks.json references gsd-check-update.js' + ); // The hook file must physically exist at the referenced path const hookFile = path.join(codexHome, 'hooks', 'gsd-check-update.js'); assert.ok( @@ -1430,11 +1452,8 @@ describe('Codex install hook configuration (e2e)', () => { const content = readCodexConfig(codexHome); assert.ok(content.includes('[features]\ncodex_hooks = true\n'), 'writes codex_hooks feature'); - // Codex 0.124.0+ nested schema: [[hooks.SessionStart]] + [[hooks.SessionStart.hooks]] const parsed = parseTomlToObject(content); - assert.ok(parsed.hooks && Array.isArray(parsed.hooks.SessionStart), 'writes [[hooks.SessionStart]] AoT'); - assert.ok(Array.isArray(parsed.hooks.SessionStart[0].hooks), 'writes [[hooks.SessionStart.hooks]] sub-table'); - assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].type, 'command', 'handler type is "command"'); + assert.ok(!parsed.hooks || !Array.isArray(parsed.hooks.SessionStart), 'config.toml does not carry managed SessionStart hooks'); // #3017: handler command now uses the absolute Node binary path so // GUI/minimal-PATH runtimes can resolve it. The shape is // "" "" @@ -1444,14 +1463,11 @@ describe('Codex install hook configuration (e2e)', () => { const expectedRunner = JSON.parse(resolveNodeRunner()); const expectedHookPath = path.join(codexHome, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); const expectedCommand = `"${expectedRunner}" "${expectedHookPath}"`; - assert.strictEqual( - parsed.hooks.SessionStart[0].hooks[0].command, - expectedCommand, - 'handler command must use absolute node runner pointing at gsd-check-update.js (#3017)' - ); - assert.ok(!Array.isArray(parsed.hooks), 'no flat [[hooks]] AoT emitted'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdCommands = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdCommands.length, 1, 'writes one GSD update hook in hooks.json'); + assert.strictEqual(gsdCommands[0], expectedCommand, 'handler command must use absolute node runner pointing at gsd-check-update.js (#3017)'); assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'writes one codex_hooks key'); - assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 1, 'writes one GSD update hook'); assertNoDraftRootKeys(content); assertUsesOnlyEol(content, '\n'); }); @@ -1533,7 +1549,9 @@ 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-check-update\.js/g), 1, 'adds one GSD update hook'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'adds one GSD update hook in hooks.json'); assertNoDraftRootKeys(content); }); @@ -1670,7 +1688,9 @@ 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-check-update\.js/g), 1, 'does not duplicate GSD update hook'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'does not duplicate GSD update hook in hooks.json'); assertNoDraftRootKeys(content); }); @@ -1712,7 +1732,9 @@ 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-check-update\.js/g), 1, 'keeps one GSD update hook'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'keeps one GSD update hook in hooks.json'); assertNoDraftRootKeys(content); }); @@ -1733,7 +1755,9 @@ 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-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'remains idempotent for the GSD hook block in hooks.json'); assertNoDraftRootKeys(content); }); @@ -1753,7 +1777,9 @@ 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-check-update\.js/g), 1, 'adds one GSD update hook for dotted codex_hooks and remains idempotent'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'adds one GSD update hook for dotted codex_hooks and remains idempotent'); assertNoDraftRootKeys(content); }); @@ -1817,7 +1843,9 @@ 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-check-update\.js/g), 1, 'adds one GSD update hook for quoted dotted codex_hooks and remains idempotent'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'adds one GSD update hook for quoted dotted codex_hooks and remains idempotent'); assertNoDraftRootKeys(content); }); @@ -1932,7 +1960,9 @@ 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-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'remains idempotent for the GSD hook block in hooks.json'); assertNoDraftRootKeys(content); }); @@ -1957,7 +1987,9 @@ 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-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'remains idempotent for the GSD hook block in hooks.json'); assertNoDraftRootKeys(content); }); @@ -1983,7 +2015,9 @@ 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-check-update\.js/g), 1, 'remains idempotent for the GSD hook block'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'remains idempotent for the GSD hook block in hooks.json'); assertNoDraftRootKeys(content); }); @@ -2028,7 +2062,9 @@ 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-check-update\.js/g), 1, 'keeps one GSD update hook'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'keeps one GSD update hook in hooks.json'); assertUsesOnlyEol(content, '\r\n'); assertNoDraftRootKeys(content); }); @@ -2051,7 +2087,9 @@ 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-check-update\.js/g), 1, 'adds the GSD update hook once'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'adds the GSD update hook once in hooks.json'); assertNoDraftRootKeys(content); }); @@ -2065,13 +2103,14 @@ describe('Codex install hook configuration (e2e)', () => { // [features] is inserted after top-level lines, before [model] — not prepended assert.ok(content.includes('# first line wins\n\n[features]\ncodex_hooks = true\n'), 'inserts features after top-level lines using first newline style'); assert.ok(content.includes(`# GSD Agent Configuration — managed by get-shit-done installer\n`), 'writes the managed agent block using the first newline style'); - // Structural check: nested schema must be present regardless of mixed EOL + // Structural check: managed SessionStart hooks live in hooks.json. const parsedMixed = parseTomlToObject(content); - assert.ok(parsedMixed.hooks && Array.isArray(parsedMixed.hooks.SessionStart), 'writes [[hooks.SessionStart]] AoT with first-newline style'); - assert.ok(Array.isArray(parsedMixed.hooks.SessionStart[0].hooks), 'writes [[hooks.SessionStart.hooks]] sub-table'); + assert.ok(!parsedMixed.hooks || !Array.isArray(parsedMixed.hooks.SessionStart), 'does not write managed SessionStart hooks to config.toml'); + const hooksJsonCommands = readHooksSessionStartCommands(codexHome); + const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js')); + assert.strictEqual(gsdEntries.length, 1, 'writes one managed SessionStart hook to hooks.json'); 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-check-update\.js/g), 1, 'does not duplicate the GSD hook block'); assertNoDraftRootKeys(content); }); }); From 3195d26d3ff2b244a55ad66e103d775f2730b216 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:45:11 -0400 Subject: [PATCH 5/7] test(codex): migrate #2698 hook assertion to hooks.json --- tests/bug-2698-crlf-install.test.cjs | 35 ++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/tests/bug-2698-crlf-install.test.cjs b/tests/bug-2698-crlf-install.test.cjs index 48e52a66d..727df7b91 100644 --- a/tests/bug-2698-crlf-install.test.cjs +++ b/tests/bug-2698-crlf-install.test.cjs @@ -66,7 +66,7 @@ describe('#2698: CRLF stale gsd-update-check block is removed on Codex reinstall // Helper: pre-populate .codex/config.toml with a GSD marker + stale hooks block // using the given line ending for the stale hooks block header, and a potentially // different EOL for the hooks body. This exercises the cross-platform mixed scenario. - function writeCodexConfigWithStaleHooks(dir, headerEol, bodyEol) { +function writeCodexConfigWithStaleHooks(dir, headerEol, bodyEol) { // Build the stale block with header EOL for the "# GSD Hooks" line, but body EOL // for the content lines (simulates a file edited by two different platforms). const staleBlock = [ @@ -92,6 +92,23 @@ describe('#2698: CRLF stale gsd-update-check block is removed on Codex reinstall return configPath; } + function readHooksSessionStartCommands(codexHome) { + const hooksPath = path.join(codexHome, 'hooks.json'); + if (!fs.existsSync(hooksPath)) return []; + const raw = fs.readFileSync(hooksPath, 'utf8').trim(); + if (!raw) return []; + const parsed = JSON.parse(raw); + const table = (parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks)) + ? parsed.hooks + : parsed; + const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : []; + return sessionStart.flatMap((entry) => + (Array.isArray(entry?.hooks) ? entry.hooks : []) + .map((hook) => hook && hook.command) + .filter((cmd) => typeof cmd === 'string') + ); + } + test('LF config.toml: stale gsd-update-check block removed on reinstall', (t) => { const origCwd = process.cwd(); t.after(() => { process.chdir(origCwd); }); @@ -107,9 +124,11 @@ describe('#2698: CRLF stale gsd-update-check block is removed on Codex reinstall !content.includes('gsd-update-check'), 'Stale gsd-update-check entry must be removed from LF config.toml (#2698)' ); - assert.ok( - content.includes('gsd-check-update'), - 'New gsd-check-update hook must appear after reinstall' + const hooksJsonCommands = readHooksSessionStartCommands(path.join(tmpDir, '.codex')); + assert.equal( + hooksJsonCommands.some((cmd) => cmd.includes('gsd-check-update')), + true, + 'New gsd-check-update hook must appear in hooks.json after reinstall' ); }); @@ -128,9 +147,11 @@ describe('#2698: CRLF stale gsd-update-check block is removed on Codex reinstall !content.includes('gsd-update-check'), 'Stale gsd-update-check entry must be removed from CRLF config.toml (#2698)' ); - assert.ok( - content.includes('gsd-check-update'), - 'New gsd-check-update hook must appear after reinstall' + const hooksJsonCommands = readHooksSessionStartCommands(path.join(tmpDir, '.codex')); + assert.equal( + hooksJsonCommands.some((cmd) => cmd.includes('gsd-check-update')), + true, + 'New gsd-check-update hook must appear in hooks.json after reinstall' ); }); From 44e65428e0ccf1c24008bc521bdd3ad14f9a98ed Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:46:34 -0400 Subject: [PATCH 6/7] test(codex): harden hooks.json sessionStart test helper --- tests/codex-config.test.cjs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 5be0a224f..a704507e0 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -91,11 +91,12 @@ function readHooksSessionStartCommands(codexHome) { ? parsed.hooks : parsed; const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : []; - return sessionStart.flatMap((entry) => - (Array.isArray(entry?.hooks) ? entry.hooks : []) - .map((hook) => hook && hook.command) - .filter((cmd) => typeof cmd === 'string') - ); + return sessionStart.flatMap((entry) => [ + ...(typeof entry?.command === 'string' ? [entry.command] : []), + ...(Array.isArray(entry?.hooks) + ? entry.hooks.map((hook) => hook && hook.command).filter((cmd) => typeof cmd === 'string') + : []), + ]); } function countMatches(content, pattern) { @@ -1432,7 +1433,11 @@ describe('Codex install hook configuration (e2e)', () => { runCodexInstall(codexHome); const configContent = readCodexConfig(codexHome); - assert.ok(!configContent.includes('gsd-check-update.js'), 'config.toml does not carry managed SessionStart hooks'); + const parsedConfig = parseTomlToObject(configContent); + assert.ok( + !parsedConfig.hooks || !Array.isArray(parsedConfig.hooks.SessionStart), + 'config.toml does not carry managed SessionStart hooks' + ); const hooksJsonCommands = readHooksSessionStartCommands(codexHome); assert.equal( hooksJsonCommands.some((cmd) => cmd.includes('gsd-check-update.js')), From e6627dfd77e76c23bc542b0ac90e7e4e89a02bfc Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 13:52:55 -0400 Subject: [PATCH 7/7] test(codex): support direct SessionStart hooks in #2698 helper --- tests/bug-2698-crlf-install.test.cjs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/bug-2698-crlf-install.test.cjs b/tests/bug-2698-crlf-install.test.cjs index 727df7b91..51f56b424 100644 --- a/tests/bug-2698-crlf-install.test.cjs +++ b/tests/bug-2698-crlf-install.test.cjs @@ -102,11 +102,12 @@ function writeCodexConfigWithStaleHooks(dir, headerEol, bodyEol) { ? parsed.hooks : parsed; const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : []; - return sessionStart.flatMap((entry) => - (Array.isArray(entry?.hooks) ? entry.hooks : []) - .map((hook) => hook && hook.command) - .filter((cmd) => typeof cmd === 'string') - ); + return sessionStart.flatMap((entry) => [ + ...(typeof entry?.command === 'string' ? [entry.command] : []), + ...(Array.isArray(entry?.hooks) + ? entry.hooks.map((hook) => hook && hook.command).filter((cmd) => typeof cmd === 'string') + : []), + ]); } test('LF config.toml: stale gsd-update-check block removed on reinstall', (t) => {