From 619915de96423f84fc9cd37ea805a05268142763 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 14 May 2026 10:43:04 -0400 Subject: [PATCH] fix(install): emit event-name leaf key for Codex AoT hooks migration (#3346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit migrateCodexHooksMapFormat re-emitted the raw `[hooks.]` path segment as the leaf TOML key of the new `[[hooks.]]` block. When the legacy table key was a `:::` location identifier and the real event lived in an `event = "..."` body field, the migration emitted a header like `[[hooks."C:\\Users\\helen\\.codex\\config.toml:session_start:0:0"]]` that Codex 0.124.0+ refuses to load — causing `npx get-shit-done-cc@latest` to abort the Codex runtime install on Windows configs that pre-date AoT. Mirror the flat-AoT branch in the map-format and stale-namespaced-AoT branches: when the section body declares `event = "..."`, that name wins as the leaf key and `event` is excluded from the re-emitted handler body. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/3346-codex-aot-toml-key.md | 5 + bin/install.js | 19 +++- tests/bug-3346-codex-aot-toml-key.test.cjs | 115 +++++++++++++++++++++ 3 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 .changeset/3346-codex-aot-toml-key.md create mode 100644 tests/bug-3346-codex-aot-toml-key.test.cjs diff --git a/.changeset/3346-codex-aot-toml-key.md b/.changeset/3346-codex-aot-toml-key.md new file mode 100644 index 000000000..c9ae3e5f6 --- /dev/null +++ b/.changeset/3346-codex-aot-toml-key.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3346 +--- +**Codex AoT hooks migration uses event-name leaf key, not location tuple** — `migrateCodexHooksMapFormat` in `bin/install.js` re-emitted the legacy `[hooks.]` section's path segment verbatim as the leaf TOML key of the new `[[hooks.]]` two-level nested AoT block. When the legacy table key was a `:::` location identifier (with the actual event name in an `event = "..."` body field), the migration produced a header like `[[hooks."C:\Users\helen\.codex\config.toml:session_start:0:0"]]`, which Codex 0.124.0+ refuses to load. The map-format and stale-namespaced-AoT branches now mirror the flat-AoT branch: when the section body declares `event = "..."`, that name wins as the leaf key (and `event` is excluded from the re-emitted handler body). `npx get-shit-done-cc@latest` no longer aborts the Codex runtime install on Windows configs that pre-date the AoT migration. (#3346) diff --git a/bin/install.js b/bin/install.js index 801a1af25..a7f14d234 100755 --- a/bin/install.js +++ b/bin/install.js @@ -3572,18 +3572,29 @@ function migrateCodexHooksMapFormat(content) { const mapOnlyBlocks = legacyMapSections .filter((s) => s.path !== 'hooks') // skip bare [hooks] container .map((s) => { - const type = s.path.slice('hooks.'.length); const body = content.slice(s.headerEnd, s.end); - return buildNestedBlock(type, body); + // #3346: when the legacy `[hooks.]` body declares `event = "..."`, + // prefer that as the event-name leaf key. The path segment may be + // a `:::` location identifier (Codex pre-AoT + // wrote those as table keys), which is not a valid leaf event name — + // emitting it verbatim produces a TOML key chain Codex 0.124.0+ rejects. + const bodyEvent = extractFlatHookEventName(body); + const type = bodyEvent !== null ? bodyEvent : s.path.slice('hooks.'.length); + const skipKeys = bodyEvent !== null ? new Set(['event']) : new Set(); + return buildNestedBlock(type, body, skipKeys); }); // Stale namespaced AoT blocks: [[hooks.TYPE]] entries with handler fields at // event-entry level (no .hooks sub-table). Treated like map-format blocks — // inserted before the first remaining table section. const staleNamespacedAotBlocks = staleNamespacedAotSections.map((s) => { - const type = s.path.slice('hooks.'.length); const body = content.slice(s.headerEnd, s.end); - return buildNestedBlock(type, body); + // #3346: see note in mapOnlyBlocks — body `event = "..."` wins over the + // raw path segment when both are present. + const bodyEvent = extractFlatHookEventName(body); + const type = bodyEvent !== null ? bodyEvent : s.path.slice('hooks.'.length); + const skipKeys = bodyEvent !== null ? new Set(['event']) : new Set(); + return buildNestedBlock(type, body, skipKeys); }); const flatAotBlocks = migratedFlatAotSections.map((s) => { diff --git a/tests/bug-3346-codex-aot-toml-key.test.cjs b/tests/bug-3346-codex-aot-toml-key.test.cjs new file mode 100644 index 000000000..5d113828a --- /dev/null +++ b/tests/bug-3346-codex-aot-toml-key.test.cjs @@ -0,0 +1,115 @@ +/** + * Regression: issue #3346 — Codex install fails on Windows when the legacy + * Codex `[hooks]` config uses a `:::` location tuple + * as the table key (with the actual event name carried in an `event = "..."` + * body field). `migrateCodexHooksMapFormat` re-emitted the location tuple + * verbatim as the leaf TOML key, producing a header like + * + * [[hooks."C:\Users\helen\.codex\config.toml:session_start:0:0"]] + * + * which Codex 0.124.0+ refuses to load (the leaf key segment is supposed to + * be the event name, not a diagnostic location identifier). + * + * Expected behaviour: when the legacy `[hooks.]` body declares an + * `event = "..."` field, the migrator must use that event name as the leaf + * TOML key for the emitted `[[hooks.]]` two-level nested AoT block. + * + * Test discipline: parse the migrated TOML with the project's own + * `parseTomlToObject` and assert on the resulting object shape — never + * grep the raw string. + */ + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); + +const { + migrateCodexHooksMapFormat, + parseTomlToObject, +} = require('../bin/install.js'); + +describe('#3346 — Codex AoT hooks migration emits event-name leaf key, not location tuple', () => { + test('legacy [hooks.""] with event="..." body migrates to [[hooks.]]', () => { + // Pre-install fixture: a legacy `[hooks.]` block whose key is + // a `:::` location identifier. The actual + // event name lives in the body as `event = "session_start"`. + const legacy = [ + '[hooks."C:\\\\Users\\\\helen\\\\.codex\\\\config.toml:session_start:0:0"]', + 'event = "session_start"', + 'command = "echo hi"', + '', + ].join('\n'); + + const migrated = migrateCodexHooksMapFormat(legacy); + const parsed = parseTomlToObject(migrated); + + // The migrated hooks object must be keyed by the event name, not by the + // location tuple. This is the core assertion of #3346. + assert.ok(parsed.hooks, 'migrated TOML must define a hooks table'); + assert.deepEqual( + Object.keys(parsed.hooks), + ['session_start'], + `migrated hooks must be keyed by event name only; got: ${JSON.stringify(Object.keys(parsed.hooks))}` + ); + + // The handler body must survive the migration and live under the two-level + // nested AoT shape (hooks.[0].hooks[0].command). + const eventEntries = parsed.hooks.session_start; + assert.ok(Array.isArray(eventEntries) && eventEntries.length >= 1, + 'hooks.session_start must be an array of tables'); + const handlers = eventEntries[0].hooks; + assert.ok(Array.isArray(handlers) && handlers.length >= 1, + 'hooks.session_start[0].hooks must be an array of handler tables'); + assert.equal(handlers[0].command, 'echo hi', + 'handler command must be preserved through migration'); + assert.equal(handlers[0].type, 'command', + 'handler type must default to "command" when no explicit type given'); + }); + + test('legacy [hooks.""] with explicit type and event survives migration cleanly', () => { + // Same as above but with an explicit `type` field — the migrator must not + // duplicate it when re-emitting the handler. + const legacy = [ + '[hooks."/home/user/.codex/config.toml:tool_call_pre:5:0"]', + 'event = "tool_call_pre"', + 'type = "command"', + 'command = "node /path/to/hook.js"', + '', + ].join('\n'); + + const migrated = migrateCodexHooksMapFormat(legacy); + const parsed = parseTomlToObject(migrated); + + assert.deepEqual( + Object.keys(parsed.hooks), + ['tool_call_pre'], + 'leaf key must be the event name from the `event = "..."` body field' + ); + const handler = parsed.hooks.tool_call_pre[0].hooks[0]; + assert.equal(handler.command, 'node /path/to/hook.js'); + assert.equal(handler.type, 'command'); + }); + + test('legacy [hooks.] without location-tuple key continues to work unchanged', () => { + // Regression guard: the fix must not break the canonical legacy-map case + // ([hooks.] with handler-fields-only body, no `event` key). + const legacy = [ + '[hooks.session_start]', + 'command = "echo hi"', + '', + ].join('\n'); + + const migrated = migrateCodexHooksMapFormat(legacy); + const parsed = parseTomlToObject(migrated); + + assert.deepEqual( + Object.keys(parsed.hooks), + ['session_start'], + 'bare-event legacy shape must continue to migrate to event-named leaf key' + ); + assert.equal(parsed.hooks.session_start[0].hooks[0].command, 'echo hi'); + }); +});