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 <noreply@anthropic.com> * chore(#1348): add changeset for Codex hooks.json canonicalization Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-17 08:51:11 -04:00
parent 07adeb50a0
commit c03f3cc6af
4 changed files with 270 additions and 13 deletions
--- a/.changeset/1348-codex-hooks-json-canonical.md
+++ 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)
--- 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<string, unknown>) : parsed;
+  // #1348: canonicalize every write to the nested { hooks: { <Event>: [...] } }
+  // 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<string, unknown> = usesNestedHooksObject
+    ? (parsed['hooks'] as Record<string, unknown>)
+    : {};
+  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;
--- 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": { "<Event>": [...] } } 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": [<user>] }, 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)}`,
+    );
+  });
+});
--- 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');