From c03f3cc6aff8ab8f61049ce627b7ffbf46acb0d2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 17 Jun 2026 08:51:11 -0400 Subject: [PATCH] fix(#1348): canonicalize Codex hooks.json writes to the nested { hooks } shape (#1363) * fix(#1348): canonicalize Codex hooks.json writes to the nested { hooks } shape reconcileCodexHooksJsonEvent preserved whatever shape it read, so on an empty, absent, or legacy top-level hooks.json it wrote top-level event keys (`{ "SessionStart": [...] }`) that current Codex (deny_unknown_fields) rejects, instead of the canonical `{ "hooks": { "SessionStart": [...] } }`. - Lift any top-level event arrays (legacy, empty, or mixed nested+top-level) into the nested `hooks` table, merging same-named events so no user/legacy entry is dropped and no stray top-level event key survives. Mirrors reconcileCursorHooksJson. - Collapse an empty hook table back to `{}` so removal on an absent file does not write a spurious `{ "hooks": {} }`. - Read path still tolerates both shapes; dedup/removal unchanged. Co-Authored-By: Claude Opus 4.8 * chore(#1348): add changeset for Codex hooks.json canonicalization Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/1348-codex-hooks-json-canonical.md | 6 + src/runtime-hooks-surface.cts | 25 +- ...codex-legacy-hooks-json-migration.test.cjs | 221 +++++++++++++++++- tests/bug-3426-codex-windows-hooks.test.cjs | 31 ++- 4 files changed, 270 insertions(+), 13 deletions(-) create mode 100644 .changeset/1348-codex-hooks-json-canonical.md diff --git a/.changeset/1348-codex-hooks-json-canonical.md b/.changeset/1348-codex-hooks-json-canonical.md new file mode 100644 index 000000000..f42285f54 --- /dev/null +++ b/.changeset/1348-codex-hooks-json-canonical.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 1363 +--- + +**Codex `hooks.json` is now always written in the nested `{ "hooks": { … } }` shape Codex expects** — the writer previously echoed back whatever shape it read, so an empty, absent, or legacy top-level `hooks.json` (`{ "SessionStart": [...] }`) stayed in the legacy shape that current Codex can reject or warn on. Every write now canonicalizes to the nested form, lifting any legacy top-level event entries (including mixed nested+top-level files) under `hooks` without dropping user-owned entries. Managed-hook dedup/removal is unchanged. (#1348) diff --git a/src/runtime-hooks-surface.cts b/src/runtime-hooks-surface.cts index f4f9035b9..62275893e 100644 --- a/src/runtime-hooks-surface.cts +++ b/src/runtime-hooks-surface.cts @@ -482,7 +482,24 @@ function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts const usesNestedHooksObject = parsed['hooks'] && typeof parsed['hooks'] === 'object' && !Array.isArray(parsed['hooks']); - const hookTable = usesNestedHooksObject ? (parsed['hooks'] as Record) : parsed; + // #1348: canonicalize every write to the nested { hooks: { : [...] } } + // shape. Lift ANY top-level event array (legacy, empty, OR mixed nested+top-level) + // into the nested table — merging when the same event exists in both — so + // user/legacy entries are preserved under `hooks` and no stray top-level event + // key survives (Codex deny_unknown_fields rejects them). Mirrors reconcileCursorHooksJson. + const hookTable: Record = usesNestedHooksObject + ? (parsed['hooks'] as Record) + : {}; + for (const key of Object.keys(parsed)) { + if (key === 'hooks') continue; + if (Array.isArray(parsed[key])) { + const lifted = parsed[key] as unknown[]; + const existing = Array.isArray(hookTable[key]) ? (hookTable[key] as unknown[]) : []; + hookTable[key] = [...lifted, ...existing]; + delete parsed[key]; + } + } + parsed['hooks'] = hookTable; const eventEntries = Array.isArray(hookTable[eventName]) ? (hookTable[eventName] as unknown[]) : []; let removedLegacy = false; @@ -524,7 +541,11 @@ function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts } else { delete hookTable[eventName]; } - if (usesNestedHooksObject) parsed['hooks'] = hookTable; + + // Avoid writing an empty `{ "hooks": {} }` artifact (e.g. removal on an absent + // file): collapse an empty hook table back to `{}` so the existing + // shouldWrite/no-write-on-empty behavior is preserved. + if (Object.keys(hookTable).length === 0) delete parsed['hooks']; const nextContent = `${JSON.stringify(parsed, null, 2)}\n`; const changed = currentContent !== nextContent; 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 aca72a615..33852a3bb 100644 --- a/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs +++ b/tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs @@ -5,6 +5,10 @@ * 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. + * + * Bug #1348 (addendum): reconcileCodexHooksJsonEvent must always write the + * canonical nested { "hooks": { "": [...] } } shape — never top-level + * event keys — mirroring reconcileCursorHooksJson. */ 'use strict'; @@ -19,7 +23,7 @@ const { execFileSync } = require('node:child_process'); const installModule = require('../bin/install.js'); const { readInstallState } = require('../gsd-core/bin/lib/installer-migrations.cjs'); -const { install, parseTomlToObject } = installModule; +const { install, parseTomlToObject, reconcileCodexHooksJsonEvent } = 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'); @@ -88,8 +92,17 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc withCodexHome(codexHome, () => install(true, 'codex')); + // #1348: output must be nested { hooks: { SessionStart: [...] } }, not top-level 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.ok( + hooksJson.hooks && typeof hooksJson.hooks === 'object' && !Array.isArray(hooksJson.hooks), + 'hooks.json must use nested { hooks: { ... } } shape (bug #1348)', + ); + assert.ok( + !Object.prototype.hasOwnProperty.call(hooksJson, 'SessionStart'), + 'hooks.json must NOT have a top-level SessionStart key (bug #1348)', + ); + const commands = hooksJson.hooks.SessionStart.flatMap((entry) => entry.hooks).map((hook) => hook.command); const managed = commands.filter((cmd) => typeof cmd === 'string' && cmd.includes('gsd-check-update')); assert.equal(managed.length, 1); assert.equal(tomlGsdHookCount(codexHome), 0); @@ -109,8 +122,17 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc withCodexHome(codexHome, () => install(true, 'codex')); + // #1348: output must be nested { hooks: { SessionStart: [...] } }, not top-level 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.ok( + hooksJson.hooks && typeof hooksJson.hooks === 'object' && !Array.isArray(hooksJson.hooks), + 'hooks.json must use nested { hooks: { ... } } shape (bug #1348)', + ); + assert.ok( + !Object.prototype.hasOwnProperty.call(hooksJson, 'SessionStart'), + 'hooks.json must NOT have a top-level SessionStart key (bug #1348)', + ); + const commands = hooksJson.hooks.SessionStart.flatMap((entry) => entry.hooks).map((hook) => hook.command); const managed = commands.filter((cmd) => typeof cmd === 'string' && cmd.includes('gsd-check-update')); 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); @@ -139,3 +161,196 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc ); }); }); + +// --------------------------------------------------------------------------- +// #1348 — reconcileCodexHooksJsonEvent must always write canonical nested shape +// --------------------------------------------------------------------------- + +describe('#1348 — reconcileCodexHooksJsonEvent canonical nested shape', { concurrency: false }, () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempDir('gsd-1348-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // (a) Fresh/absent hooks.json: register → { "hooks": { "SessionStart": [...] } } + test('(a) fresh/absent hooks.json writes nested { hooks: { SessionStart: [...] } } shape', () => { + const hooksJsonPath = path.join(tmpDir, 'hooks.json'); + const FAKE_CMD = `"/usr/local/bin/node" "${path.join(tmpDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/')}"`; + assert.ok(!fs.existsSync(hooksJsonPath), 'precondition: hooks.json must not exist'); + + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: FAKE_CMD }); + + assert.ok(fs.existsSync(hooksJsonPath), 'hooks.json must be created'); + const hooksJson = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); + + assert.ok( + hooksJson.hooks && typeof hooksJson.hooks === 'object' && !Array.isArray(hooksJson.hooks), + `Expected nested { hooks: { ... } } shape; got: ${JSON.stringify(hooksJson)}`, + ); + assert.ok( + !Object.prototype.hasOwnProperty.call(hooksJson, 'SessionStart'), + `hooks.json must NOT have a top-level SessionStart key; got: ${JSON.stringify(hooksJson)}`, + ); + assert.ok( + Array.isArray(hooksJson.hooks.SessionStart) && hooksJson.hooks.SessionStart.length > 0, + `Expected hooks.hooks.SessionStart to be a non-empty array; got: ${JSON.stringify(hooksJson)}`, + ); + }); + + // (b) Legacy migration: seed top-level { "SessionStart": [] }, register → + // nested hooks.SessionStart contains BOTH migrated user entry AND managed entry + test('(b) legacy top-level shape: user entries migrate into hooks.SessionStart alongside managed entry', () => { + const FAKE_CMD = `"/usr/local/bin/node" "${path.join(tmpDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/')}"`; + const userEntry = { hooks: [{ type: 'command', command: 'node "/Users/alice/my-hook.js"' }] }; + fs.writeFileSync( + path.join(tmpDir, 'hooks.json'), + JSON.stringify({ SessionStart: [userEntry] }, null, 2), + ); + + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: FAKE_CMD }); + + const hooksJson = JSON.parse(fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8')); + + // Canonical nested shape + assert.ok( + hooksJson.hooks && typeof hooksJson.hooks === 'object' && !Array.isArray(hooksJson.hooks), + `Expected nested { hooks: { ... } } shape; got: ${JSON.stringify(hooksJson)}`, + ); + assert.ok( + !Object.prototype.hasOwnProperty.call(hooksJson, 'SessionStart'), + `hooks.json must NOT have a top-level SessionStart key; got: ${JSON.stringify(hooksJson)}`, + ); + + // User entry was migrated under hooks.SessionStart (not dropped) + const allCommands = hooksJson.hooks.SessionStart + .flatMap((e) => Array.isArray(e.hooks) ? e.hooks : []) + .map((h) => h.command); + assert.ok( + allCommands.includes('node "/Users/alice/my-hook.js"'), + `User entry must be preserved under hooks.SessionStart; commands: ${JSON.stringify(allCommands)}`, + ); + + // Managed entry is also present + const managedCount = allCommands.filter((c) => typeof c === 'string' && c.includes('gsd-check-update')).length; + assert.equal(managedCount, 1, 'Exactly one managed entry must be present under hooks.SessionStart'); + }); + + // (c-i) Dedup: re-registering the same managed command does not duplicate it + test('(c-i) re-registering managed command produces exactly one managed entry', () => { + const FAKE_CMD = `"/usr/local/bin/node" "${path.join(tmpDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/')}"`; + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: FAKE_CMD }); + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: FAKE_CMD }); + + const hooksJson = JSON.parse(fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8')); + const allCommands = hooksJson.hooks.SessionStart + .flatMap((e) => Array.isArray(e.hooks) ? e.hooks : []) + .map((h) => h.command); + const managedCount = allCommands.filter((c) => typeof c === 'string' && c.includes('gsd-check-update')).length; + assert.equal(managedCount, 1, 'Re-register must yield exactly one managed entry'); + }); + + // (c-ii) Removal: user entries remain under hooks, managed entry is gone + test('(c-ii) removing managed hook leaves user entry under hooks.SessionStart', () => { + const FAKE_CMD = `"/usr/local/bin/node" "${path.join(tmpDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/')}"`; + const userEntry = { hooks: [{ type: 'command', command: 'node "/Users/alice/my-hook.js"' }] }; + // Seed already-nested file with both user + managed + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: FAKE_CMD }); + // Now manually seed a user entry into the existing nested file + const seeded = JSON.parse(fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8')); + seeded.hooks.SessionStart = [userEntry, ...seeded.hooks.SessionStart]; + fs.writeFileSync(path.join(tmpDir, 'hooks.json'), JSON.stringify(seeded, null, 2)); + + // Remove managed + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: null }); + + const hooksJson = JSON.parse(fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8')); + // User entry must still be under hooks.SessionStart + const allCommands = hooksJson.hooks.SessionStart + .flatMap((e) => Array.isArray(e.hooks) ? e.hooks : []) + .map((h) => h.command); + assert.ok( + allCommands.includes('node "/Users/alice/my-hook.js"'), + `User entry must remain after managed removal; commands: ${JSON.stringify(allCommands)}`, + ); + // No managed entry + const managedCount = allCommands.filter((c) => typeof c === 'string' && c.includes('gsd-check-update')).length; + assert.equal(managedCount, 0, 'No managed entry must remain after removal'); + }); + + // (c-iii) Removal from absent file does NOT materialize { "hooks": {} } + test('(c-iii) removing from absent hooks.json does not write a spurious empty { "hooks": {} }', () => { + const hooksJsonPath = path.join(tmpDir, 'hooks.json'); + assert.ok(!fs.existsSync(hooksJsonPath), 'precondition: hooks.json must not exist'); + + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: null }); + + assert.ok( + !fs.existsSync(hooksJsonPath), + 'hooks.json must NOT be created when removing from absent file (no spurious { "hooks": {} })', + ); + }); + + // (d) Mixed nested + top-level shape: { "hooks": { "PreToolUse": [...] }, "SessionStart": [...] } + // The stray top-level event array must be lifted into hooks and merged; no top-level key survives. + test('(d) mixed nested + top-level shape: stray top-level event array is lifted and merged', () => { + const FAKE_CMD = `"/usr/local/bin/node" "${path.join(tmpDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/')}"`; + const existingNestedEntry = { hooks: [{ type: 'command', command: 'node "/Users/alice/pre-tool.js"' }] }; + const userTopLevelEntry = { hooks: [{ type: 'command', command: 'node "/Users/alice/session-start.js"' }] }; + + // Seed a mixed-shape file: nested PreToolUse AND top-level SessionStart + fs.writeFileSync( + path.join(tmpDir, 'hooks.json'), + JSON.stringify( + { + hooks: { PreToolUse: [existingNestedEntry] }, + SessionStart: [userTopLevelEntry], + }, + null, + 2, + ), + ); + + reconcileCodexHooksJsonEvent(tmpDir, 'SessionStart', { managedCommand: FAKE_CMD }); + + const hooksJson = JSON.parse(fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8')); + + // No stray top-level SessionStart key + assert.ok( + !Object.prototype.hasOwnProperty.call(hooksJson, 'SessionStart'), + `hooks.json must NOT have a top-level SessionStart key; got: ${JSON.stringify(hooksJson)}`, + ); + + // hooks.SessionStart contains the migrated user entry AND exactly one managed entry + assert.ok( + Array.isArray(hooksJson.hooks.SessionStart), + `hooks.hooks.SessionStart must be an array; got: ${JSON.stringify(hooksJson)}`, + ); + const sessionCommands = hooksJson.hooks.SessionStart + .flatMap((e) => Array.isArray(e.hooks) ? e.hooks : []) + .map((h) => h.command); + assert.ok( + sessionCommands.includes('node "/Users/alice/session-start.js"'), + `Migrated user entry must be present in hooks.SessionStart; commands: ${JSON.stringify(sessionCommands)}; full: ${JSON.stringify(hooksJson)}`, + ); + const managedCount = sessionCommands.filter((c) => typeof c === 'string' && c.includes('gsd-check-update')).length; + assert.equal(managedCount, 1, `Exactly one managed entry must be present in hooks.SessionStart; commands: ${JSON.stringify(sessionCommands)}`); + + // hooks.PreToolUse is untouched + assert.ok( + Array.isArray(hooksJson.hooks.PreToolUse) && hooksJson.hooks.PreToolUse.length === 1, + `hooks.hooks.PreToolUse must be preserved with one entry; got: ${JSON.stringify(hooksJson.hooks.PreToolUse)}`, + ); + const preToolCommands = hooksJson.hooks.PreToolUse + .flatMap((e) => Array.isArray(e.hooks) ? e.hooks : []) + .map((h) => h.command); + assert.ok( + preToolCommands.includes('node "/Users/alice/pre-tool.js"'), + `Existing nested PreToolUse entry must be preserved; commands: ${JSON.stringify(preToolCommands)}`, + ); + }); +}); diff --git a/tests/bug-3426-codex-windows-hooks.test.cjs b/tests/bug-3426-codex-windows-hooks.test.cjs index a36585dab..3a05b1966 100644 --- a/tests/bug-3426-codex-windows-hooks.test.cjs +++ b/tests/bug-3426-codex-windows-hooks.test.cjs @@ -57,6 +57,21 @@ const { const { projectManagedHookCommand } = PROJECTION; +/** + * Extract hook handler objects for `eventName` from a hooks.json object. + * Handles both the legacy top-level shape { SessionStart: [...] } and the + * canonical nested shape { hooks: { SessionStart: [...] } } (bug #1348). + */ +function hookHandlersForEvent(hooksJson, eventName) { + if (!hooksJson || typeof hooksJson !== 'object') return []; + const table = + hooksJson.hooks && typeof hooksJson.hooks === 'object' && !Array.isArray(hooksJson.hooks) + ? hooksJson.hooks + : hooksJson; + if (!Array.isArray(table[eventName])) return []; + return table[eventName].flatMap((e) => Array.isArray(e && e.hooks) ? e.hooks : []); +} + // ─── Step 1: Export surface check ──────────────────────────────────────────── describe('#3426 — export surface: buildCodexHookWindowsShimIR must be exported', () => { @@ -255,8 +270,8 @@ describe('#3426 integration: ensureCodexHooksJsonSessionStart on win32 writes .c assert.ok(fs.existsSync(hooksJsonPath), 'hooks.json must exist after install'); const hooksJson = JSON.parse(fs.readFileSync(hooksJsonPath, 'utf8')); - const commands = (hooksJson.SessionStart || []) - .flatMap((e) => (Array.isArray(e.hooks) ? e.hooks : [])) + // #1348: hooks.json is now always written in nested { hooks: { ... } } shape + const commands = hookHandlersForEvent(hooksJson, 'SessionStart') .map((h) => h && h.command) .filter((c) => typeof c === 'string'); @@ -307,8 +322,8 @@ describe('#3426 integration: ensureCodexHooksJsonSessionStart on win32 writes .c const hooksJson = JSON.parse( fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8'), ); - const commands = (hooksJson.SessionStart || []) - .flatMap((e) => (Array.isArray(e.hooks) ? e.hooks : [])) + // #1348: hooks.json is now always written in nested { hooks: { ... } } shape + const commands = hookHandlersForEvent(hooksJson, 'SessionStart') .map((h) => h && h.command) .filter((c) => typeof c === 'string'); @@ -344,8 +359,8 @@ describe('#3426 integration: ensureCodexHooksJsonSessionStart on win32 writes .c const hooksJson = JSON.parse( fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8'), ); - const commands = (hooksJson.SessionStart || []) - .flatMap((e) => (Array.isArray(e.hooks) ? e.hooks : [])) + // #1348: hooks.json is now always written in nested { hooks: { ... } } shape + const commands = hookHandlersForEvent(hooksJson, 'SessionStart') .map((h) => h && h.command) .filter((c) => typeof c === 'string'); @@ -440,8 +455,8 @@ describe('#3426 upgrade: reinstall on win32 migrates existing "node script.js" t }); const hooksJson = JSON.parse(fs.readFileSync(path.join(tmpDir, 'hooks.json'), 'utf8')); - const commands = (hooksJson.SessionStart || []) - .flatMap((e) => (Array.isArray(e.hooks) ? e.hooks : [])) + // #1348: hooks.json is now always written in nested { hooks: { ... } } shape + const commands = hookHandlersForEvent(hooksJson, 'SessionStart') .map((h) => h && h.command) .filter((c) => typeof c === 'string');