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); + }); +});