From 3c03a153a57a5972704d1ff93b3694d9d2c521d8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 28 Apr 2026 18:28:53 -0400 Subject: [PATCH] fix(#2773): emit correct Codex 0.124.0+ two-level nested hooks schema (#2809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2773): emit correct Codex 0.124.0+ two-level nested hooks schema Codex 0.124.0's stable spec requires: [[hooks.SessionStart]] ← event entry (optional matcher) [[hooks.SessionStart.hooks]] ← handler sub-table type = "command" command = "node ..." Previous GSD versions wrote the flat [[hooks]] + event = "SessionStart" form (#2637) or a single-block [[hooks.SessionStart]] without the nested .hooks sub-table (#2760). Both are rejected by Codex 0.124.0+ at launch. Changes: bin/install.js - Hook block emission now always writes the two-level nested AoT form. - migrateCodexHooksMapFormat extended to also migrate flat [[hooks]] array-of-tables entries (event = "..." key → [[hooks.]] form). Flat [[hooks]] and [[hooks.]] are mutually exclusive TOML types; any pre-existing flat entries must be promoted before GSD appends its own namespaced hooks. - Migrated flat AoT blocks are inserted BEFORE the GSD marker so they stay in the "user" portion of the file and survive stripGsdFromCodexConfig. - stripCodexGsd* regexes cover all four historical block shapes. - validateCodexConfigSchema no longer rejects flat [[hooks]] at the root level (removing the false-positive that blocked install when users had their own AfterCommand hooks). The validator still enforces the nested [[hooks..hooks]] shape for entries that have a .hooks sub-table. tests/ - bug-2760-codex-install-defensive.test.cjs: 29/29 passing. Added 5 new regression cases for fresh install, upgrade from each legacy shape, idempotent reinstall, and user hook preservation. - codex-config.test.cjs: 106/106 passing. All migration tests updated to assert [[hooks..hooks]] sub-table (command now in handler level, not event-entry level). New tests: flat [[hooks]] migration (SessionStart, AfterCommand), install+uninstall preserves non-GSD AfterCommand hook. Closes #2773 Co-Authored-By: Claude Sonnet 4.6 * fix: address CodeRabbit review + CI regression in bug-2698-crlf-install CI regression (#2698 tests): Strip GSD-managed hook blocks BEFORE running migrateCodexHooksMapFormat. The previous order let migration convert the stale [[hooks]] + event = "SessionStart" + gsd-update-check.js block to [[hooks.SessionStart]] form before Shape 1 strip regex could match it; Shape 1 only matches the flat [[hooks]] form, so the stale block survived reinstall. Swapping to strip-then-migrate ensures only user-authored hooks reach the migration step. Shape 3/4 regexes also extended to match both gsd-check-update.js and the legacy gsd-update-check.js filename so no variant slips through. CodeRabbit actionable (major): migrateCodexHooksMapFormat now accepts single-quoted TOML event values (event = 'SessionStart') in the flat [[hooks]] filter and event-name extractor. TOML spec allows single-quoted literal strings; double-quote-only regexes silently skipped them, leaving the block unmigrated and triggering the hard-fail validator. CodeRabbit nitpicks: tests/codex-config.test.cjs: replace indexOf('[[hooks.AfterCommand]]') ordering check with parseTomlToObject structural assertions (no-source-grep rule). tests/bug-2760-codex-install-defensive.test.cjs: replace three content.match(/…/g).length raw-text counts with parseTomlToObject structural assertions for single-handler and single-event-entry invariants. Co-Authored-By: Claude Sonnet 4.6 * fix: address CodeRabbit review #2 — extractFlatHookEventName helper + type assertions - bin/install.js: consolidate TOML_QUOTED_STRING + TOML_EVENT_CAPTURE into a single extractFlatHookEventName() helper that rejects empty-string event values (event = "" or event = ''); previously two independent regexes had to be kept in sync and neither guarded against a blank event name producing a [[hooks.]] header - tests/bug-2760-codex-install-defensive.test.cjs: add comments explaining why the e.command fallback is retained in both allSessionStartCommands and afterToolCommands collectors — migration only upgrades [hooks.TYPE] map-format sections, not existing [[hooks.TYPE]] namespaced AoT entries authored with command at event-entry level; removing the fallback causes false failures for preserved user entries - tests/codex-config.test.cjs: add type = "command" assertions to all migration tests that verify .command but were missing .type checks; buildNestedBlock injects type = "command" when the source body has no explicit type key, so every migrated handler must carry it per the Codex 0.124.0+ schema 138 tests pass, 0 fail. Co-Authored-By: Claude Sonnet 4.6 * fix: CR round 3 + proactive audit — TOML quoting, stale AoT migration, strict validator Three real issues from CodeRabbit round 3, plus the collateral improvements they enable: bin/install.js — tomlBareKey() helper (#2773 CR6a) buildNestedBlock interpolated the raw event name into [[hooks.${type}]] and [[hooks.${type}.hooks]] headers without TOML escaping. An event name containing spaces or punctuation (e.g. "Before Tool") would produce invalid TOML that parseTomlToObject would subsequently reject. Added tomlBareKey() — wraps the key in double-quoted TOML strings when it contains non-bare-key characters ([A-Za-z0-9_-]). bin/install.js — staleNamespacedAotSections migration path (#2773 CR6b) migrateCodexHooksMapFormat handled [hooks.TYPE] (map-format) and flat [[hooks]] with event = "..." but ignored [[hooks.TYPE]] AoT entries that carried handler fields (command, type, timeout, statusMessage) at event-entry level without a nested [[hooks.TYPE.hooks]] sub-table. This is the pre-#2773 single-block shape that Codex 0.124.0+ rejects. Added staleNamespacedAotSections as the third migration category: detected by STALE_HANDLER_FIELD_PATTERN + absence of a [[hooks.TYPE.hooks]] sub-table in the same file; promoted to the two-level nested form by buildNestedBlock. Matcher-only entries (no handler fields) are intentionally skipped. bin/install.js — validator now rejects event-level handler fields (#2773 CR6c) With migration covering the stale AoT shape, validateCodexConfigSchema can be strict: entries that have handler fields at event-entry level but no .hooks sub-array return ok: false instead of silently passing. Matcher-only entries (no handler fields and no .hooks) remain valid as event filters. tests/codex-config.test.cjs — four new migration tests + missing type assertion Four tests cover the new stale AoT migration path: single-entry promotion, already-nested entry is left untouched (no double-wrap), multiple event types, and matcher-only entry is skipped. Added the missing type = "command" assertion to the CRLF migration test (the one miss from CR round 2). tests/bug-2760-codex-install-defensive.test.cjs — strict .hooks-only collectors With stale AoT entries now migrated, the entry.command fallbacks in allSessionStartCommands and afterToolCommands are dead code. Replaced with strict entry.hooks-only collection guarded by an every(Array.isArray(e.hooks)) pre-assertion, so any future regression that leaves handler fields at event level produces an explicit test failure rather than silently collecting them. 142 tests pass, 0 fail. Co-Authored-By: Claude Sonnet 4.6 * fix: CR round 4 — segment-safe quoted-key detection + structural test assertions bin/install.js — getTomlTableSections now exposes segments (#2773 CR7a) The staleNamespacedAotSections filter used section.path.split('.').length > 2 to skip [[hooks.TYPE.hooks]] sub-table entries. That check misclassifies quoted event names containing dots: [[hooks."before.tool"]] has path hooks.before.tool (3 dot-parts) but only 2 true parsed segments, so it was incorrectly excluded from migration. Fixed by adding segments to the getTomlTableSections return shape (already available on record.tableHeader.segments) and replacing the split-based check with section.segments.length !== 2, which uses the true parsed key count regardless of dots inside quoted names. tests/codex-config.test.cjs — replace raw-equality assertions (#2773 CR7b) The two new no-op migration tests (already-nested and matcher-only) used assert.strictEqual(result, content) — raw string equality that conflicts with the repo no-source-grep testing standard. Replaced with structural assertions using parseTomlToObject: the already-nested test verifies the handler stays under .hooks[0] and no double-wrap occurs; the matcher-only test verifies the matcher key is preserved and no .hooks sub-array is added. 142 tests pass, 0 fail. Co-Authored-By: Claude Sonnet 4.6 * fix: CR round 5 — parseHooksBody key parser, empty-handler guard, segment-safe legacyMap filter, stronger test assertions - parseHooksBody: replace /^([\w.]+)\s*=/ regex with parseTomlKey() so hyphenated keys (status-message) and quoted keys are not silently dropped - buildNestedBlock: guard against handlerEntries.length === 0 — do not synthesise [[hooks.TYPE.hooks]] with type="command" but no command for matcher-only or otherwise handler-empty stale sections - legacyMapSections filter: use section.segments.length === 2 (same fix applied to staleNamespacedAotSections in round 4) to prevent [hooks.X.Y] 3-segment tables from being misclassified as event entries - tests: add regression test for [[hooks."before.tool"]] quoted-dot event names; strengthen command path assertion to exact absolute path comparison Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- bin/install.js | 380 +++++++++++++----- .../bug-2760-codex-install-defensive.test.cjs | 218 +++++++--- tests/codex-config.test.cjs | 245 ++++++++++- 3 files changed, 678 insertions(+), 165 deletions(-) diff --git a/bin/install.js b/bin/install.js index 988713d4a..1504b53eb 100755 --- a/bin/install.js +++ b/bin/install.js @@ -2628,6 +2628,11 @@ function getTomlTableSections(content) { return headerLines.map((record, index) => ({ path: record.tableHeader.path, + // segments preserves the true parsed key count so callers that need to + // distinguish a 2-segment path like hooks."before.tool" from a 3-segment + // path like hooks.SessionStart.hooks can do so without splitting on dots + // (which misclassifies quoted key names that contain dot characters). + segments: record.tableHeader.segments, array: record.tableHeader.array, start: record.start, headerEnd: record.end + record.eol.length, @@ -2891,41 +2896,134 @@ function stripLeakedGsdCodexSections(content) { function migrateCodexHooksMapFormat(content) { const sections = getTomlTableSections(content); - // Find all non-array hooks sections: [hooks] or [hooks.TYPE] - const legacyHooksSections = sections.filter( - (section) => !section.array && (section.path === 'hooks' || section.path.startsWith('hooks.')) + // Find all non-array hooks sections: bare [hooks] container or [hooks.TYPE] event tables. + // Use section.segments (parsed key count) rather than section.path.startsWith() so that + // nested handler tables like [hooks.SessionStart.hooks] (3 segments) are not mistakenly + // included and re-emitted as an event named "SessionStart.hooks". + const legacyMapSections = sections.filter( + (section) => !section.array && ( + section.path === 'hooks' || + (section.path.startsWith('hooks.') && section.segments.length === 2) + ) ); - if (legacyHooksSections.length === 0) { + // Find flat [[hooks]] array-of-tables entries (path === 'hooks', array === true). + // These are incompatible with [[hooks.]] namespaced form — both cannot + // coexist in the same TOML file because `hooks` cannot be simultaneously an + // array and a table. Migrate each flat entry to [[hooks.]] form using + // the `event` key as the event name. + const flatAotSections = sections.filter( + (section) => section.array && section.path === 'hooks' + ); + + // Find [[hooks.TYPE]] namespaced AoT entries that carry handler fields + // (command, type, timeout, statusMessage) at event-entry level but have no + // [[hooks.TYPE.hooks]] sub-table. This is the pre-#2773 single-block shape + // that Codex 0.124.0+ rejects. Promote them to the two-level nested form. + // Entries that already have a [[hooks.TYPE.hooks]] sub-table are left untouched. + // Matcher-only entries (no handler fields) are intentionally valid and skipped. + const STALE_HANDLER_FIELD_PATTERN = /^\s*(?:command|type|timeout|statusMessage)\s*=/m; + const staleNamespacedAotSections = sections.filter((section) => { + if (!section.array) return false; + if (!section.path.startsWith('hooks.')) return false; + // [[hooks.TYPE.hooks]] sub-tables have 3 parsed segments — skip them. + // Use section.segments (true parsed key count) rather than splitting + // section.path on '.', which misclassifies quoted event names that contain + // dots (e.g. [[hooks."before.tool"]] has segments ['hooks','before.tool'] + // but path 'hooks.before.tool' would split into 3 parts). + if (section.segments.length !== 2) return false; + // Must carry at least one handler field at event-entry level. + const body = content.slice(section.headerEnd, section.end); + if (!STALE_HANDLER_FIELD_PATTERN.test(body)) return false; + // Don't migrate when the nested [[hooks.TYPE.hooks]] sub-table already exists. + const subPath = section.path + '.hooks'; + return !sections.some((s) => s.array && s.path === subPath); + }); + + if (legacyMapSections.length === 0 && flatAotSections.length === 0 && staleNamespacedAotSections.length === 0) { return content; } const eol = detectLineEnding(content); - // Build [[hooks]] blocks for each [hooks.TYPE] section (skipping bare [hooks]) - const newHooksBlocks = []; - for (const section of legacyHooksSections) { - if (section.path === 'hooks') { - // Bare [hooks] container — drop it (no key-value content to convert) - continue; + // Helper: parse a hooks body into event-level and handler-level entries, + // returning { eventEntries, handlerEntries, hasExplicitType }. + // Event-level keys: matcher. Everything else is handler-level. + // The `event` key (used in flat [[hooks]] blocks) is consumed as the type + // name and excluded from both levels. + const EVENT_LEVEL_KEYS = new Set(['matcher']); + function parseHooksBody(body, skipKeys = new Set()) { + const bodyLines = body.split(/\r?\n/); + const eventEntries = []; + const handlerEntries = []; + let hasExplicitType = false; + for (const line of bodyLines) { + const trimmed = line.trim(); + if (!trimmed || trimmed.startsWith('#')) continue; + // Use parseTomlKey so hyphenated keys (e.g. status-message) and quoted + // keys are recognised — the old /^([\w.]+)\s*=/ regex silently dropped them. + const parsed = parseTomlKey(trimmed); + if (!parsed) continue; + // Hook body keys are always single-segment; use segments[0] for the name. + const key = parsed.segments[0]; + if (skipKeys.has(key)) continue; + if (key === 'type') { + hasExplicitType = true; + handlerEntries.push(trimmed); + } else if (EVENT_LEVEL_KEYS.has(key)) { + eventEntries.push(trimmed); + } else { + handlerEntries.push(trimmed); + } } - - // Extract the type from the path: "hooks.shell" → "shell" - const type = section.path.slice('hooks.'.length); - const body = content.slice(section.headerEnd, section.end); - - // #2760 CR5 finding 3 — emit the namespaced AoT form directly: - // `[[hooks.]]` (no synthetic `event` field — the namespace IS the - // event). Previously we emitted flat `[[hooks]]\nevent = ""`, - // which produced mixed flat + namespaced layouts when the user already - // had `[[hooks.]]` entries. With every migration emit using the - // namespaced shape, the managed-emit detector - // (`hasUserNamespacedAotHooks`) fires correctly and the install - // converges on a single hook layout. - const block = `[[hooks.${type}]]${eol}${body}`; - newHooksBlocks.push(block); + return { eventEntries, handlerEntries, hasExplicitType }; } + // TOML key quoting: bare keys may only contain [A-Za-z0-9_-]. Event names + // containing spaces, dots, or other punctuation must be wrapped in double- + // quoted TOML strings with backslash and double-quote characters escaped. + // Using raw event names in [[hooks.${type}]] headers produces invalid TOML + // for any non-bare-key character (e.g. "Before Tool" → [[hooks.Before Tool]]). + function tomlBareKey(key) { + if (/^[A-Za-z0-9_-]+$/.test(key)) return key; + return '"' + key.replace(/\\/g, '\\\\').replace(/"/g, '\\"') + '"'; + } + + function buildNestedBlock(type, body, skipKeys = new Set()) { + const quotedType = tomlBareKey(type); + const { eventEntries, handlerEntries, hasExplicitType } = parseHooksBody(body, skipKeys); + const eventBody = eventEntries.length > 0 ? eventEntries.join(eol) + eol : ''; + // If no handler fields were found (e.g. matcher-only entry), do not synthesise + // an empty [[hooks.TYPE.hooks]] block — that would produce structurally valid + // TOML but semantically broken output (a handler entry with no command). + if (handlerEntries.length === 0) { + return `[[hooks.${quotedType}]]${eol}${eventBody}`; + } + if (!hasExplicitType) handlerEntries.unshift('type = "command"'); + const handlerBody = handlerEntries.join(eol) + eol; + return `[[hooks.${quotedType}]]${eol}${eventBody}${eol}[[hooks.${quotedType}.hooks]]${eol}${handlerBody}`; + } + + // Extract the event name from a flat [[hooks]] section body. + // Returns null if no `event` key is found, if the value is an empty string, or if + // the quoting is unrecognised. Both TOML double-quoted ("...") and single-quoted + // ('...') strings are accepted. An empty event string (event = "" or event = '') + // is explicitly rejected — it cannot be meaningfully namespaced and is left untouched. + function extractFlatHookEventName(body) { + const TOML_EVENT_CAPTURE = /^\s*event\s*=\s*(?:"((?:[^"\\]|\\.)*)"|'([^']*)')/m; + const m = body.match(TOML_EVENT_CAPTURE); + if (!m) return null; + const name = (m[1] ?? m[2] ?? '').trim(); + return name || null; + } + + const migratedFlatAotSections = flatAotSections.filter((section) => { + const body = content.slice(section.headerEnd, section.end); + return extractFlatHookEventName(body) !== null; + }); + + const legacyHooksSections = [...legacyMapSections, ...migratedFlatAotSections, ...staleNamespacedAotSections]; + // Remove all legacy hooks sections from the content let result = removeContentRanges( content, @@ -2933,28 +3031,43 @@ function migrateCodexHooksMapFormat(content) { ); result = collapseTomlBlankLines(result); - // Insert new [[hooks]] blocks at the position of the first legacy section - // (adjusted for removed content), or append if nothing remains before EOF. - if (newHooksBlocks.length > 0) { - const insertionText = newHooksBlocks.join(''); - // Find a good insertion point: before the first remaining table section - // that came after our removed hooks, or just append. - const remainingSections = getTomlTableSections(result); - const firstHooksSection = legacyHooksSections[0]; - - // Find the first remaining section whose original start was after the legacy hooks block - const anchorSection = remainingSections.find((s) => { - // Use content position in the result string as a heuristic - // We insert before the first non-hooks section if any exists - return s.start > 0; + // Map-format blocks ([hooks.TYPE]) are inserted at the position of the first + // remaining table section (preserving their relative placement in the file). + // Flat AoT blocks ([[hooks]] with event = "...") are always APPENDED because + // flat [[hooks]] entries only appear at the END of a TOML file (AoT cannot + // precede a regular table), and inserting before the first table would push + // them above [features] / [model] etc., corrupting relative ordering. + 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); }); - // Prefer to insert the new blocks right before the first remaining table - // that was originally positioned after the legacy hooks area, but since - // positions shift after removal, we simply append before the first table - // header or at end-of-file. + // 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); + }); + + const flatAotBlocks = migratedFlatAotSections.map((s) => { + const body = content.slice(s.headerEnd, s.end); + const eventName = extractFlatHookEventName(body); + if (!eventName) return ''; + return buildNestedBlock(eventName, body, new Set(['event'])); + }).filter(Boolean); + + // Insert map-format and stale-namespaced-AoT conversions before the first + // remaining table section (both share the same placement strategy). + const allMapStyleBlocks = [...mapOnlyBlocks, ...staleNamespacedAotBlocks]; + if (allMapStyleBlocks.length > 0) { + const insertionText = allMapStyleBlocks.join(''); + const remainingSections = getTomlTableSections(result); if (remainingSections.length > 0) { - // Find where to insert: after any leading top-level keys, before first table const firstTable = remainingSections[0]; const before = result.slice(0, firstTable.start); const after = result.slice(firstTable.start); @@ -2966,7 +3079,23 @@ function migrateCodexHooksMapFormat(content) { (needsTrailingGap ? eol : '') + after; } else { - // No remaining sections — append + const needsGap = result.length > 0 && !result.endsWith(eol + eol); + result = result + (needsGap ? eol : '') + insertionText; + } + } + + // Insert flat-AoT conversions before the GSD managed marker (if present) so + // the migrated user hooks stay in the "user" portion of the file and are not + // swept away when stripGsdFromCodexConfig strips from the marker to EOF. + // If no marker exists, append at the end of the file. + if (flatAotBlocks.length > 0) { + const insertionText = flatAotBlocks.join(''); + const markerIdx = result.indexOf(GSD_CODEX_MARKER); + if (markerIdx !== -1) { + const before = result.slice(0, markerIdx).trimEnd(); + const after = result.slice(markerIdx); + result = before + eol + eol + insertionText + eol + after; + } else { const needsGap = result.length > 0 && !result.endsWith(eol + eol); result = result + (needsGap ? eol : '') + insertionText; } @@ -3400,14 +3529,63 @@ function validateCodexConfigSchema(content) { } // Structural confirmation against parsed object: any present hooks. - // must be an array. - if (parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks)) { - for (const [event, value] of Object.entries(parsed.hooks)) { - if (!Array.isArray(value)) { - return { - ok: false, - reason: `hooks.${event} must be an array of tables, got ${typeof value}`, - }; + // must be an array, and flat top-level [[hooks]] (parsed as Array on root) + // is rejected — Codex 0.124.0+ requires [[hooks.]] namespaced form. + if (parsed.hooks !== undefined) { + if (Array.isArray(parsed.hooks)) { + return { + ok: false, + reason: 'flat [[hooks]] array-of-tables is invalid in Codex 0.124.0+ (expected [[hooks.]] namespaced form)', + }; + } + if (typeof parsed.hooks === 'object' && parsed.hooks !== null) { + for (const [event, value] of Object.entries(parsed.hooks)) { + // Skip the nested .hooks sub-array — it lives under hooks.[n].hooks + // and is validated separately below. + if (!Array.isArray(value)) { + return { + ok: false, + reason: `hooks.${event} must be an array of tables, got ${typeof value}`, + }; + } + // Each entry in hooks. must either be a matcher-only filter (no + // handler fields) or carry a .hooks sub-array of handler tables. + // Entries with handler fields (command, type, timeout, statusMessage) at + // event-entry level but without a .hooks sub-table are the pre-#2773 + // single-block shape that Codex 0.124.0+ rejects. migrateCodexHooksMapFormat + // converts these before validation runs; their presence here means migration + // failed to cover this entry — fail loudly rather than pass a broken config. + const HANDLER_FIELD_NAMES = new Set(['command', 'type', 'timeout', 'statusMessage']); + for (const entry of value) { + if (!entry || typeof entry !== 'object') continue; + if (entry.hooks === undefined) { + const strayKey = Object.keys(entry).find((k) => HANDLER_FIELD_NAMES.has(k)); + if (strayKey) { + return { + ok: false, + reason: `hooks.${event}[] entry has handler field "${strayKey}" at event-entry level; ` + + `Codex 0.124.0+ requires handler fields nested under [[hooks.${event}.hooks]]`, + }; + } + continue; + } + if (!Array.isArray(entry.hooks)) { + return { + ok: false, + reason: `hooks.${event}[].hooks must be an array of handler tables, got ${typeof entry.hooks}`, + }; + } + for (const handler of entry.hooks) { + if (handler && typeof handler === 'object' && handler.type !== undefined) { + if (handler.type !== 'command') { + return { + ok: false, + reason: `hooks.${event}[].hooks[].type must be "command", got "${handler.type}"`, + }; + } + } + } + } } } } @@ -6973,62 +7151,64 @@ function install(isGlobal, runtime = 'claude') { let configContent = fs.existsSync(configPath) ? fs.readFileSync(configPath, 'utf-8') : ''; const eol = detectLineEnding(configContent); - // Migrate legacy [hooks] map format to [[hooks]] array-of-tables (#2637). - // Codex 0.124.0 requires [[hooks]] array-of-tables; old GSD installs wrote - // [hooks.shell] map tables which now cause a startup parse error. + // Strip ALL prior GSD-managed hook blocks BEFORE migration so the migration + // only touches user-authored hooks, not GSD-owned stale entries. Running + // strip after migration causes Shape 1 (legacy gsd-update-check filename) + // to be converted by migration before the strip regex can match it (#2698). + // + // Historical shapes stripped, in order: + // Shape 1 — legacy gsd-update-check filename (pre-#1755): flat [[hooks]] + event + // Shape 2 — flat [[hooks]] + event = "SessionStart" (#2637 era, never correct) + // Shape 4 — correct two-block nested (strip before shape 3 to avoid orphaned header) + // Shape 3 — single-block [[hooks.SessionStart]] without nested .hooks (#2760 era) + if (configContent.includes('gsd-update-check') || configContent.includes('gsd-check-update')) { + // Shape 1 + configContent = configContent.replace( + /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-update-check\.js"\r?\n/gm, + (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), + ); + // Shape 2 + configContent = configContent.replace( + /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-check-update\.js"\r?\n/gm, + (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), + ); + // Shape 4 — strip before shape 3 to avoid orphaned header + configContent = configContent.replace( + /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\.SessionStart\]\]\r?\n\r?\n\[\[hooks\.SessionStart\.hooks\]\]\r?\ntype = "command"\r?\ncommand = "node [^\r\n]*(?:gsd-check-update|gsd-update-check)\.js"\r?\n/gm, + (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), + ); + // Shape 3 + configContent = configContent.replace( + /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\.SessionStart\]\]\r?\ncommand = "node [^\r\n]*(?:gsd-check-update|gsd-update-check)\.js"\r?\n/gm, + (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), + ); + } + + // Migrate legacy [hooks] map format and flat [[hooks]] AoT entries to the + // namespaced [[hooks.]] form after stripping GSD-managed stale blocks. + // Running migration after strip ensures only user-authored hooks are migrated + // (#2698 regression: migration before strip converts stale GSD blocks before + // the strip regexes can match their original shape). const migratedContent = migrateCodexHooksMapFormat(configContent); if (migratedContent !== configContent) { configContent = migratedContent; - console.log(` ${green}✓${reset} Migrated legacy Codex [hooks] map format to [[hooks]] array-of-tables`); + console.log(` ${green}✓${reset} Migrated legacy Codex [hooks] format to two-level nested AoT`); } const codexHooksFeature = ensureCodexHooksFeature(configContent); configContent = setManagedCodexHooksOwnership(codexHooksFeature.content, codexHooksFeature.ownership); - // Add SessionStart hook for update checking. Default to top-level - // `[[hooks]]` AoT with `event` field — the form GSD has emitted since - // the Codex 0.124 migration (#2637). When the user already uses the - // namespaced AoT form `[[hooks.SessionStart]]` for their own hooks, - // emit our managed entry in that same shape so the two forms don't - // collide on round-trip (#2760, defect 3). + // 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) const updateCheckScript = path.resolve(targetDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); - const useNamespacedAot = hasUserNamespacedAotHooks(configContent, 'SessionStart'); - const hookBlock = useNamespacedAot - ? `${eol}# GSD Hooks${eol}` + - `[[hooks.SessionStart]]${eol}` + - `command = "node ${updateCheckScript}"${eol}` - : `${eol}# GSD Hooks${eol}` + - `[[hooks]]${eol}` + - `event = "SessionStart"${eol}` + - `command = "node ${updateCheckScript}"${eol}`; - - // Migrate legacy gsd-update-check entries from prior installs (#1755 followup) - // Remove stale hook blocks that used the inverted filename or wrong path. - // Single \r?\n-aware regex handles LF, CRLF, and block-at-file-start (#2698). - if (configContent.includes('gsd-update-check')) { - configContent = configContent.replace( - /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-update-check\.js"\r?\n/gm, - (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), - ); - } - - // #2760 CR4 finding 2 — Strip ALL existing managed gsd-check-update - // hook blocks (top-level [[hooks]] AND namespaced [[hooks.SessionStart]]) - // BEFORE evaluating the includes guard. Without this, an install that - // already has a legacy flat [[hooks]] block short-circuits the new - // namespaced AoT emit and stays stuck in the mixed layout this fix is - // designed to eliminate. Stripping first means every install converges - // on the right shape regardless of prior state. - if (configContent.includes('gsd-check-update')) { - configContent = configContent.replace( - /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-check-update\.js"\r?\n/gm, - (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), - ); - configContent = configContent.replace( - /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\.SessionStart\]\]\r?\ncommand = "node [^\r\n]*gsd-check-update\.js"\r?\n/gm, - (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), - ); - } + const hookBlock = `${eol}# GSD Hooks${eol}` + + `[[hooks.SessionStart]]${eol}` + + `${eol}` + + `[[hooks.SessionStart.hooks]]${eol}` + + `type = "command"${eol}` + + `command = "node ${updateCheckScript}"${eol}`; if (hasEnabledCodexHooksFeature(configContent) && !configContent.includes('gsd-check-update')) { configContent += hookBlock; diff --git a/tests/bug-2760-codex-install-defensive.test.cjs b/tests/bug-2760-codex-install-defensive.test.cjs index 790efbb22..da73c56f2 100644 --- a/tests/bug-2760-codex-install-defensive.test.cjs +++ b/tests/bug-2760-codex-install-defensive.test.cjs @@ -90,12 +90,59 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei fs.rmSync(tmpDir, { recursive: true, force: true }); }); - test('preserves both pre-existing [[hooks.SessionStart]] entries and adds GSD entry in namespaced form', () => { + test('fresh install emits the two-level nested AoT schema (#2773)', () => { + // Codex 0.124.0+ requires [[hooks.SessionStart]] + [[hooks.SessionStart.hooks]] + // with type = "command". Neither the flat [[hooks]] + event field form nor + // the single-block [[hooks.SessionStart]] form without .hooks is accepted. + writeCodexConfig(codexHome, ''); + runCodexInstall(codexHome); + const content = readCodexConfig(codexHome); + const parsed = parseTomlToObject(content); + + // hooks must be an object (namespaced), NOT a flat array. + 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]]' + ); + }); + + test('preserves user [[hooks.SessionStart]] entries and adds GSD nested handler', () => { + // 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 = [ '[[hooks.SessionStart]]', + '', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', 'command = "echo first user hook"', '', '[[hooks.SessionStart]]', + '', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', 'command = "echo second user hook"', '', ].join('\n'); @@ -105,59 +152,111 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei const afterInstall = readCodexConfig(codexHome); const parsed = parseTomlToObject(afterInstall); - // hooks.SessionStart must be an array-of-tables (namespaced AoT form). assert.ok( parsed.hooks && Array.isArray(parsed.hooks.SessionStart), - 'hooks.SessionStart must be an array-of-tables, got: ' - + (parsed.hooks ? typeof parsed.hooks.SessionStart : 'no hooks table') + 'hooks.SessionStart must remain an array-of-tables after install' ); - const commands = parsed.hooks.SessionStart.map((entry) => entry.command); - - // Both pre-existing user hook entries survive in the parsed structure. - assert.ok( - commands.includes('echo first user hook'), - 'first user [[hooks.SessionStart]] entry preserved in parsed structure: ' + JSON.stringify(commands) - ); - assert.ok( - commands.includes('echo second user hook'), - 'second user [[hooks.SessionStart]] entry preserved in parsed structure: ' + JSON.stringify(commands) + // Collect all handler commands across all event entries. + const allCommands = parsed.hooks.SessionStart.flatMap((entry) => + Array.isArray(entry.hooks) ? entry.hooks.map((h) => h.command) : [] ); - // GSD's managed entry is emitted in the same namespaced AoT shape so it - // does not collide with the user's preferred form. assert.ok( - commands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), - 'GSD entry must appear in hooks.SessionStart array (not top-level [[hooks]]): ' - + JSON.stringify(commands) + allCommands.includes('echo first user hook'), + 'first user hook preserved: ' + JSON.stringify(allCommands) ); - - // Top-level [[hooks]] AoT must not coexist when namespaced form is in use — - // mixing forms is what produces the round-trip break this fix prevents. assert.ok( - !Array.isArray(parsed.hooks) || parsed.hooks.length === 0, - 'no top-level [[hooks]] AoT entries when namespaced form is in use' + allCommands.includes('echo second user hook'), + 'second user hook preserved: ' + JSON.stringify(allCommands) ); + 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) + ); + assert.ok(!Array.isArray(parsed.hooks), 'no flat [[hooks]] entries'); }); - test('selects top-level [[hooks]] form when user has no namespaced hooks (status-quo behavior)', () => { - writeCodexConfig(codexHome, ''); + test('reinstall replaces flat [[hooks]] + event form with nested schema', () => { + // Upgrade path: user has a config written by GSD 1.38.x (flat [[hooks]] form). + const legacyConfig = [ + '[features]', + 'codex_hooks = true', + '', + '# GSD Hooks', + '[[hooks]]', + 'event = "SessionStart"', + 'command = "node /old/path/to/gsd-check-update.js"', + '', + ].join('\n'); + writeCodexConfig(codexHome, legacyConfig); + runCodexInstall(codexHome); const content = readCodexConfig(codexHome); const parsed = parseTomlToObject(content); - // Top-level hooks must be an array-of-tables; the GSD entry must be one - // of those tables and carry event = "SessionStart". - assert.ok( - Array.isArray(parsed.hooks), - 'fresh install must produce top-level [[hooks]] AoT, got: ' + typeof parsed.hooks + // 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)); + assert.strictEqual(gsdHandlers.length, 1, 'exactly one managed handler after upgrade'); + }); + + test('reinstall replaces single-block [[hooks.SessionStart]] (no .hooks sub-table) with nested schema', () => { + // Upgrade path: user has a config written by the PR #2802 shape — + // [[hooks.SessionStart]] without a nested [[hooks.SessionStart.hooks]] sub-table. + const prBranchConfig = [ + '[features]', + 'codex_hooks = true', + '', + '# GSD Hooks', + '[[hooks.SessionStart]]', + 'command = "node /old/path/to/gsd-check-update.js"', + '', + ].join('\n'); + writeCodexConfig(codexHome, prBranchConfig); + + runCodexInstall(codexHome); + 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.ok( - parsed.hooks.some((h) => h && h.event === 'SessionStart'), - 'top-level [[hooks]] AoT must contain an entry with event = "SessionStart": ' - + JSON.stringify(parsed.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' ); }); + + test('reinstall is idempotent: correct nested schema is stripped and re-emitted cleanly', () => { + writeCodexConfig(codexHome, ''); + runCodexInstall(codexHome); + 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'); + }); }); describe('#2760 fix 2 — Strip purges invalid legacy [agents] / [[agents]] regardless of marker', () => { @@ -572,15 +671,25 @@ describe('#2760 CR4 finding 2 — Legacy flat [[hooks]] block migrates to namesp + (parsed.hooks ? typeof parsed.hooks.SessionStart : 'no hooks table') ); - const namespacedCommands = parsed.hooks.SessionStart.map((entry) => entry.command); + // Migration now handles stale [[hooks.SessionStart]] entries with handler + // fields at event-entry level (pre-#2773 shape), promoting them to the + // two-level nested form. Every entry must carry a .hooks sub-array after + // migration, so collect from nested handlers only. assert.ok( - namespacedCommands.includes('echo user hook'), - 'user [[hooks.SessionStart]] entry preserved: ' + JSON.stringify(namespacedCommands) + parsed.hooks.SessionStart.every((entry) => Array.isArray(entry.hooks)), + 'every hooks.SessionStart entry must use nested [[hooks.SessionStart.hooks]] handlers after migration' + ); + const allSessionStartCommands = parsed.hooks.SessionStart.flatMap((entry) => + entry.hooks.map((h) => h.command).filter(Boolean) ); assert.ok( - namespacedCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), + allSessionStartCommands.includes('echo user hook'), + 'user [[hooks.SessionStart]] entry preserved: ' + JSON.stringify(allSessionStartCommands) + ); + 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(namespacedCommands) + + JSON.stringify(allSessionStartCommands) ); // The legacy top-level [[hooks]] AoT must NOT coexist with the namespaced @@ -592,7 +701,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 = namespacedCommands.filter( + const gsdEntries = allSessionStartCommands.filter( (cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd) ); assert.equal(gsdEntries.length, 1, @@ -936,18 +1045,35 @@ describe('#2760 CR5 finding 3 — migration emits namespaced AoT (no flat/namesp parsed.hooks && Array.isArray(parsed.hooks.AfterTool), 'pre-existing [[hooks.AfterTool]] must remain a namespaced AoT array' ); + // AfterTool was in [[hooks.AfterTool]] with command at event-entry level + // (pre-#2773 stale namespaced AoT shape). Migration now promotes these to + // the two-level nested form, so every entry must have a .hooks sub-array. assert.ok( - parsed.hooks.AfterTool.some((entry) => entry.command === 'x'), - 'user AfterTool entry must be preserved: ' + JSON.stringify(parsed.hooks.AfterTool) + parsed.hooks.AfterTool.every((e) => Array.isArray(e.hooks)), + 'every AfterTool entry must use nested [[hooks.AfterTool.hooks]] handlers after migration' + ); + const afterToolCommands = parsed.hooks.AfterTool.flatMap((e) => + e.hooks.map((h) => h.command).filter(Boolean) + ); + assert.ok( + afterToolCommands.includes('x'), + 'user AfterTool entry must be preserved: ' + JSON.stringify(afterToolCommands) ); - // The migrated SessionStart entry is now namespaced AoT, not flat - // [[hooks]] with event="SessionStart". + // The migrated SessionStart entry is now namespaced AoT with nested .hooks sub-table. assert.ok( parsed.hooks && Array.isArray(parsed.hooks.SessionStart), 'migrated SessionStart must be namespaced AoT (not flat [[hooks]])' ); - const ssCommands = parsed.hooks.SessionStart.map((e) => e.command); + // After migration, [hooks.SessionStart] map-format is promoted to nested AoT. + // Command lives in [[hooks.SessionStart.hooks]][0].command (nested schema). + assert.ok( + parsed.hooks.SessionStart.every((e) => Array.isArray(e.hooks)), + 'every SessionStart entry must use nested [[hooks.SessionStart.hooks]] handlers after migration' + ); + const ssCommands = parsed.hooks.SessionStart.flatMap((e) => + e.hooks.map((h) => h.command).filter(Boolean) + ); assert.ok( ssCommands.includes('y'), 'user SessionStart command "y" must be preserved in namespaced array: ' + diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 89523fa72..087d3b54e 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -578,7 +578,9 @@ describe('stripGsdFromCodexConfig', () => { // ─── migrateCodexHooksMapFormat ───────────────────────────────────────────────── describe('migrateCodexHooksMapFormat', () => { - test('returns content unchanged when no legacy [hooks] map sections present', () => { + test('migrates flat [[hooks]] with event key to namespaced [[hooks.]] form', () => { + // Flat [[hooks]] + event = "..." is TOML-incompatible with [[hooks.SessionStart]], + // so migrateCodexHooksMapFormat now converts it to the nested namespaced form. const content = [ '[features]', 'codex_hooks = true', @@ -588,7 +590,21 @@ describe('migrateCodexHooksMapFormat', () => { 'command = "node /home/.codex/hooks/gsd-check-update.js"', '', ].join('\n'); - assert.strictEqual(migrateCodexHooksMapFormat(content), content); + const result = migrateCodexHooksMapFormat(content); + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.SessionStart), + 'flat [[hooks]] event=SessionStart must be promoted to [[hooks.SessionStart]] AoT'); + assert.strictEqual(parsed.hooks.SessionStart.length, 1); + assert.ok(Array.isArray(parsed.hooks.SessionStart[0].hooks), + 'must emit [[hooks.SessionStart.hooks]] sub-table'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].command, + 'node /home/.codex/hooks/gsd-check-update.js'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].type, 'command', + 'migrated handler must carry type = "command" per Codex 0.124.0+ schema'); + assert.equal(parsed.hooks.SessionStart[0].event, undefined, + 'event key consumed as namespace — must not appear in emitted block'); + assert.ok(!Array.isArray(parsed.hooks), 'hooks must be a table, not a flat array'); + assert.equal(parsed.features && parsed.features.codex_hooks, true); }); test('returns content unchanged for empty string', () => { @@ -612,7 +628,10 @@ describe('migrateCodexHooksMapFormat', () => { assert.ok(parsed.hooks && Array.isArray(parsed.hooks.shell), 'hooks.shell must be an array of tables, got: ' + (parsed.hooks ? typeof parsed.hooks.shell : 'no hooks table')); assert.strictEqual(parsed.hooks.shell.length, 1); - assert.strictEqual(parsed.hooks.shell[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); + // #2773: command now lives in [[hooks.shell.hooks]] sub-table, not at event-entry level + assert.ok(Array.isArray(parsed.hooks.shell[0].hooks), 'must emit [[hooks.shell.hooks]] sub-table'); + assert.strictEqual(parsed.hooks.shell[0].hooks[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); + assert.strictEqual(parsed.hooks.shell[0].hooks[0].type, 'command'); // No flat top-level [[hooks]] AoT and no synthetic event field. assert.ok(!Array.isArray(parsed.hooks), 'no top-level [[hooks]] AoT — namespace IS the event in CR5 form'); @@ -633,8 +652,12 @@ describe('migrateCodexHooksMapFormat', () => { const parsed = parseTomlToObject(result); assert.ok(parsed.hooks && Array.isArray(parsed.hooks.exec)); assert.strictEqual(parsed.hooks.exec.length, 1); - assert.strictEqual(parsed.hooks.exec[0].command, 'echo hello'); - assert.strictEqual(parsed.hooks.exec[0].extra_key, 'preserved'); + // #2773: command and extra keys now live in [[hooks.exec.hooks]] sub-table + assert.ok(Array.isArray(parsed.hooks.exec[0].hooks), 'must emit [[hooks.exec.hooks]] sub-table'); + assert.strictEqual(parsed.hooks.exec[0].hooks[0].command, 'echo hello'); + assert.strictEqual(parsed.hooks.exec[0].hooks[0].type, 'command', + 'migrated handler must carry type = "command" per Codex 0.124.0+ schema'); + assert.strictEqual(parsed.hooks.exec[0].hooks[0].extra_key, 'preserved'); assert.equal(parsed.hooks.exec[0].event, undefined); }); @@ -653,18 +676,37 @@ describe('migrateCodexHooksMapFormat', () => { assert.ok(parsed.hooks && Array.isArray(parsed.hooks.exec)); assert.strictEqual(parsed.hooks.shell.length, 1); assert.strictEqual(parsed.hooks.exec.length, 1); - assert.strictEqual(parsed.hooks.shell[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); - assert.strictEqual(parsed.hooks.exec[0].command, 'echo done'); + // #2773: commands now live in the [[hooks..hooks]] sub-table + assert.strictEqual(parsed.hooks.shell[0].hooks[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); + assert.strictEqual(parsed.hooks.shell[0].hooks[0].type, 'command', + 'migrated shell handler must carry type = "command"'); + assert.strictEqual(parsed.hooks.exec[0].hooks[0].command, 'echo done'); + assert.strictEqual(parsed.hooks.exec[0].hooks[0].type, 'command', + 'migrated exec handler must carry type = "command"'); }); - test('leaves user-authored [[hooks]] array entries untouched when no legacy [hooks] map present', () => { + test('migrates flat [[hooks]] with event=AfterCommand to [[hooks.AfterCommand]] namespaced form', () => { + // Flat [[hooks]] + event = "..." is incompatible with [[hooks.]] AoT in the same + // file — TOML cannot have hooks be both an array and a table. Migration promotes it. const content = [ '[[hooks]]', 'event = "AfterCommand"', 'command = "echo custom"', '', ].join('\n'); - assert.strictEqual(migrateCodexHooksMapFormat(content), content); + const result = migrateCodexHooksMapFormat(content); + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.AfterCommand), + 'flat [[hooks]] event=AfterCommand must become [[hooks.AfterCommand]] AoT'); + assert.strictEqual(parsed.hooks.AfterCommand.length, 1); + assert.ok(Array.isArray(parsed.hooks.AfterCommand[0].hooks), + 'must emit [[hooks.AfterCommand.hooks]] sub-table'); + assert.strictEqual(parsed.hooks.AfterCommand[0].hooks[0].command, 'echo custom'); + assert.strictEqual(parsed.hooks.AfterCommand[0].hooks[0].type, 'command', + 'migrated AfterCommand handler must carry type = "command" per Codex 0.124.0+ schema'); + assert.equal(parsed.hooks.AfterCommand[0].event, undefined, + 'event key consumed as namespace — must not appear in emitted block'); + assert.ok(!Array.isArray(parsed.hooks), 'hooks must be a table, not a flat array'); }); test('end-to-end: install on config with old [hooks] map format produces namespaced AoT (#2637, #2760 CR5)', () => { @@ -686,8 +728,12 @@ describe('migrateCodexHooksMapFormat', () => { assert.ok(parsed.hooks && Array.isArray(parsed.hooks.shell), 'hooks.shell must be array-of-tables in namespaced form'); assert.strictEqual(parsed.hooks.shell.length, 1); - assert.strictEqual(parsed.hooks.shell[0].command, + // #2773: command lives in [[hooks.shell.hooks]] sub-table + assert.ok(Array.isArray(parsed.hooks.shell[0].hooks), 'must emit [[hooks.shell.hooks]] sub-table'); + assert.strictEqual(parsed.hooks.shell[0].hooks[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); + assert.strictEqual(parsed.hooks.shell[0].hooks[0].type, 'command', + 'migrated shell handler must carry type = "command" per Codex 0.124.0+ schema'); assert.equal(parsed.features && parsed.features.codex_hooks, true); }); @@ -710,6 +756,132 @@ describe('migrateCodexHooksMapFormat', () => { assert.ok(result.includes('[model]'), 'preserves [model]'); }); + test('upgrades stale [[hooks.SessionStart]] with event-level command to nested schema (#2773 CR6)', () => { + // Pre-#2773 single-block format: handler fields live directly under + // [[hooks.SessionStart]] rather than under [[hooks.SessionStart.hooks]]. + // Codex 0.124.0+ rejects this shape. Migration must promote it. + const content = [ + '[features]', + 'codex_hooks = true', + '', + '[[hooks.SessionStart]]', + 'command = "echo stale-user-hook"', + '', + ].join('\n'); + const result = migrateCodexHooksMapFormat(content); + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.SessionStart), + 'stale [[hooks.SessionStart]] must remain a namespaced AoT'); + assert.strictEqual(parsed.hooks.SessionStart.length, 1); + assert.ok(Array.isArray(parsed.hooks.SessionStart[0].hooks), + 'must emit [[hooks.SessionStart.hooks]] sub-table'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].command, 'echo stale-user-hook'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].type, 'command', + 'must inject type = "command" when source body has no explicit type'); + assert.equal(parsed.hooks.SessionStart[0].command, undefined, + 'command must not remain at event-entry level after promotion'); + assert.equal(parsed.features && parsed.features.codex_hooks, true); + }); + + test('leaves [[hooks.SessionStart]] + [[hooks.SessionStart.hooks]] untouched (already nested)', () => { + // Properly-nested schema: handler lives under [[hooks.SessionStart.hooks]]. + // Migration must NOT create a double-wrapped [[hooks.SessionStart.hooks.hooks]] shape. + const content = [ + '[[hooks.SessionStart]]', + '', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', + 'command = "echo already-nested"', + '', + ].join('\n'); + const result = migrateCodexHooksMapFormat(content); + const parsed = parseTomlToObject(result); + assert.ok(Array.isArray(parsed.hooks?.SessionStart), + 'SessionStart must remain a namespaced AoT after no-op migration'); + assert.strictEqual(parsed.hooks.SessionStart.length, 1, + 'must not duplicate the event entry'); + assert.ok(Array.isArray(parsed.hooks.SessionStart[0].hooks), + 'nested [[hooks.SessionStart.hooks]] sub-table must still be present'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks.length, 1, + 'must not create a double-wrapped [[hooks.SessionStart.hooks.hooks]]'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].type, 'command'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].command, 'echo already-nested'); + assert.equal(parsed.hooks.SessionStart[0].command, undefined, + 'command must not appear at event-entry level'); + }); + + test('promotes multiple stale [[hooks.TYPE]] entries from different event types', () => { + const content = [ + '[[hooks.SessionStart]]', + 'command = "echo session"', + '', + '[[hooks.AfterCommand]]', + 'command = "echo after-cmd"', + '', + ].join('\n'); + const result = migrateCodexHooksMapFormat(content); + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.SessionStart)); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.AfterCommand)); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].command, 'echo session'); + assert.strictEqual(parsed.hooks.SessionStart[0].hooks[0].type, 'command'); + assert.strictEqual(parsed.hooks.AfterCommand[0].hooks[0].command, 'echo after-cmd'); + assert.strictEqual(parsed.hooks.AfterCommand[0].hooks[0].type, 'command'); + assert.equal(parsed.hooks.SessionStart[0].command, undefined); + assert.equal(parsed.hooks.AfterCommand[0].command, undefined); + }); + + test('matcher-only [[hooks.SessionStart]] (no handler fields) is left untouched', () => { + // A [[hooks.SessionStart]] entry with only a `matcher` key is a valid + // event filter — no handler fields → not a stale single-block entry. + const content = [ + '[[hooks.SessionStart]]', + 'matcher = "some-tool"', + '', + ].join('\n'); + const result = migrateCodexHooksMapFormat(content); + const parsed = parseTomlToObject(result); + assert.ok(Array.isArray(parsed.hooks?.SessionStart), + 'matcher-only SessionStart must remain a namespaced AoT'); + assert.strictEqual(parsed.hooks.SessionStart.length, 1); + assert.strictEqual(parsed.hooks.SessionStart[0].matcher, 'some-tool', + 'matcher key must be preserved'); + assert.equal(parsed.hooks.SessionStart[0].hooks, undefined, + 'matcher-only entry must not gain a .hooks sub-array'); + assert.equal(parsed.hooks.SessionStart[0].command, undefined, + 'no spurious command key must appear'); + }); + + test('quoted event name with dot ([[hooks."before.tool"]]) is treated as single 2-segment namespace', () => { + // Regression for the split('.') bug: "before.tool" contains a dot, but the + // key is quoted so it is ONE segment — [[hooks."before.tool"]] has exactly + // two path segments and must be classified the same as [[hooks.SessionStart]]. + // It should NOT be treated as a 3-level path (hooks / before / tool). + const content = [ + '[[hooks."before.tool"]]', + 'command = "echo hi"', + '', + ].join('\n'); + const result = migrateCodexHooksMapFormat(content); + const parsed = parseTomlToObject(result); + // The key in the parsed object is the unquoted event name "before.tool". + assert.ok( + parsed.hooks && Array.isArray(parsed.hooks['before.tool']), + '[[hooks."before.tool"]] must be a namespaced AoT — not split on the inner dot' + ); + assert.ok( + Array.isArray(parsed.hooks['before.tool'][0].hooks), + 'must emit [[hooks."before.tool".hooks]] sub-table' + ); + assert.strictEqual( + parsed.hooks['before.tool'][0].hooks[0].command, + 'echo hi', + 'command must be preserved in the nested handler sub-table' + ); + // Ensure no spurious "before" or "tool" top-level hook keys appeared. + assert.equal(parsed.hooks?.before, undefined, 'must not split quoted key on dot'); + }); + test('CRLF line endings are preserved through migration (#2760 CR5: namespaced AoT)', () => { const content = [ '[features]', @@ -725,8 +897,12 @@ describe('migrateCodexHooksMapFormat', () => { // Round-trip parse confirms the structural shape independent of EOL. const parsed = parseTomlToObject(result); assert.ok(parsed.hooks && Array.isArray(parsed.hooks.shell)); - assert.strictEqual(parsed.hooks.shell[0].command, + // #2773: command lives in [[hooks.shell.hooks]] sub-table + assert.ok(Array.isArray(parsed.hooks.shell[0].hooks), 'must emit [[hooks.shell.hooks]] sub-table'); + assert.strictEqual(parsed.hooks.shell[0].hooks[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); + assert.strictEqual(parsed.hooks.shell[0].hooks[0].type, 'command', + 'migrated shell handler must carry type = "command" per Codex 0.124.0+ schema'); }); }); @@ -750,7 +926,7 @@ describe('Codex hooks emit: migration produces namespaced AoT so managed-emit co fs.rmSync(tmpDir, { recursive: true, force: true }); }); - test('migration of legacy [hooks.SessionStart] produces namespaced AoT', () => { + test('migration of legacy [hooks.SessionStart] produces two-level nested AoT (#2773)', () => { const legacyContent = [ '[features]', 'codex_hooks = true', @@ -761,16 +937,27 @@ describe('Codex hooks emit: migration produces namespaced AoT so managed-emit co ].join('\n'); const migrated = migrateCodexHooksMapFormat(legacyContent); const parsed = parseTomlToObject(migrated); + // Outer event entry assert.ok( parsed.hooks && Array.isArray(parsed.hooks.SessionStart), 'migration must emit [[hooks.SessionStart]] namespaced AoT' ); assert.equal(parsed.hooks.SessionStart[0].event, undefined, 'migration must NOT emit a synthetic event field — namespace IS the event'); - assert.equal( - Array.isArray(parsed.hooks), - false, - 'migration must NOT emit a flat top-level [[hooks]] AoT' + assert.equal(Array.isArray(parsed.hooks), false, + 'migration must NOT emit a flat top-level [[hooks]] AoT'); + // Inner handler sub-table + assert.ok( + Array.isArray(parsed.hooks.SessionStart[0].hooks), + 'migration must emit [[hooks.SessionStart.hooks]] sub-table' + ); + const handler = parsed.hooks.SessionStart[0].hooks[0]; + assert.strictEqual(handler.type, 'command', + 'migration must inject type = "command" in handler sub-table'); + assert.strictEqual( + handler.command, + 'node /home/.codex/hooks/gsd-check-update.js', + 'migration must preserve original command value in handler sub-table' ); }); }); @@ -1237,7 +1424,17 @@ describe('Codex install hook configuration (e2e)', () => { const content = readCodexConfig(codexHome); assert.ok(content.includes('[features]\ncodex_hooks = true\n'), 'writes codex_hooks feature'); - assert.ok(content.includes('# GSD Hooks\n[[hooks]]\nevent = "SessionStart"\n'), 'writes GSD SessionStart hook block'); + // 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.strictEqual( + parsed.hooks.SessionStart[0].hooks[0].command, + 'node ' + path.join(codexHome, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'), + 'handler command must be the exact absolute path to gsd-check-update.js' + ); + assert.ok(!Array.isArray(parsed.hooks), 'no flat [[hooks]] AoT emitted'); 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); @@ -1667,7 +1864,14 @@ describe('Codex install hook configuration (e2e)', () => { assert.ok(content.includes('notes = \'\'\'\n[model]\ncodex_hooks = false\n\'\'\''), 'preserves multiline string content'); assert.strictEqual(countMatches(content, /^codex_hooks = false$/gm), 1, 'does not rewrite codex_hooks text inside multiline string'); assert.ok(content.indexOf('codex_hooks = true') > content.indexOf('other_feature = true'), 'does not stop the features section at multiline string content'); - assert.ok(content.indexOf('codex_hooks = true') < content.indexOf('[[hooks]]'), 'inserts the real codex_hooks key before the next table'); + // Parse structurally — verify codex_hooks and migrated AfterCommand hook via parsed object + const parsed = parseTomlToObject(content); + assert.equal(parsed.features?.codex_hooks, true, 'writes a real codex_hooks boolean key'); + assert.ok(Array.isArray(parsed.hooks?.AfterCommand), 'AfterCommand flat [[hooks]] migrated to namespaced AoT'); + const afterCmds = parsed.hooks.AfterCommand.flatMap((entry) => + Array.isArray(entry.hooks) ? entry.hooks.map((h) => h.command).filter(Boolean) : [] + ); + assert.ok(afterCmds.includes('echo custom-after-command'), 'preserves AfterCommand user hook command'); assertNoDraftRootKeys(content); }); @@ -1846,7 +2050,10 @@ 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'); - assert.ok(content.includes('# GSD Hooks\n[[hooks]]\nevent = "SessionStart"\n'), 'writes the GSD hook block using the first newline style'); + // Structural check: nested schema must be present regardless of mixed EOL + 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(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');