diff --git a/bin/install.js b/bin/install.js index 988713d4a5..a4e3c9b305 100755 --- a/bin/install.js +++ b/bin/install.js @@ -2977,16 +2977,16 @@ function migrateCodexHooksMapFormat(content) { /** * Detect whether the user already uses the namespaced AoT hooks form - * (`[[hooks.]]`) for the given event in the config. When true, + * (`[[hooks.]]`) in the config. When true, * the GSD-managed hook block must be emitted in the same shape so it * coexists cleanly — mixing `[[hooks]]` (flat) with `[[hooks.SessionStart]]` * (namespaced) in the same file confuses round-trip writers and can * produce a config that Codex rejects (#2760, defect 3). */ -function hasUserNamespacedAotHooks(content, event) { +function hasUserNamespacedAotHooks(content) { const sections = getTomlTableSections(content); return sections.some( - (section) => section.array && section.path === `hooks.${event}` + (section) => section.array && section.path.startsWith('hooks.') ); } @@ -6988,19 +6988,21 @@ function install(isGlobal, runtime = 'claude') { // 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, + // namespaced AoT form `[[hooks.]]` 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). + // + // #2802 review finding: sense style from user content ONLY by stripping + // previous GSD-managed blocks first. + const userContent = stripGsdFromCodexConfig(configContent) || ''; 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}`; + const hookBlock = `${eol}# GSD Hooks${eol}` + + (hasUserNamespacedAotHooks(userContent) + ? `[[hooks.SessionStart]]${eol}` + + `command = "node ${updateCheckScript}"${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. diff --git a/tests/bug-2760-codex-install-defensive.test.cjs b/tests/bug-2760-codex-install-defensive.test.cjs index 790efbb22d..701e092e83 100644 --- a/tests/bug-2760-codex-install-defensive.test.cjs +++ b/tests/bug-2760-codex-install-defensive.test.cjs @@ -349,7 +349,7 @@ describe('#2760 — hasUserNamespacedAotHooks helper', () => { 'command = "x"', '', ].join('\n'); - assert.equal(hasUserNamespacedAotHooks(content, 'SessionStart'), true); + assert.equal(hasUserNamespacedAotHooks(content), true); }); test('returns false when only top-level [[hooks]] entries exist', () => { @@ -359,7 +359,7 @@ describe('#2760 — hasUserNamespacedAotHooks helper', () => { 'command = "x"', '', ].join('\n'); - assert.equal(hasUserNamespacedAotHooks(content, 'SessionStart'), false); + assert.equal(hasUserNamespacedAotHooks(content), false); }); test('returns false when only single-bracket [hooks.SessionStart] exists', () => { @@ -368,7 +368,7 @@ describe('#2760 — hasUserNamespacedAotHooks helper', () => { 'command = "x"', '', ].join('\n'); - assert.equal(hasUserNamespacedAotHooks(content, 'SessionStart'), false); + assert.equal(hasUserNamespacedAotHooks(content), false); }); }); diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 89523fa72f..bb4c214c00 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -1236,8 +1236,16 @@ describe('Codex install hook configuration (e2e)', () => { runCodexInstall(codexHome); 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'); + const parsed = parseTomlToObject(content); + assert.strictEqual(parsed.features && parsed.features.codex_hooks, true, 'writes codex_hooks feature'); + assert.ok( + Array.isArray(parsed.hooks), + 'fresh install must produce top-level [[hooks]] AoT, got: ' + typeof parsed.hooks + ); + assert.ok( + parsed.hooks.some((h) => h && h.event === 'SessionStart' && h.command.includes('gsd-check-update.js')), + 'top-level [[hooks]] AoT must contain the GSD SessionStart entry' + ); 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); @@ -1846,7 +1854,15 @@ 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'); + const parsed = parseTomlToObject(content); + assert.ok( + Array.isArray(parsed.hooks), + 'fresh install must produce top-level [[hooks]] AoT, got: ' + typeof parsed.hooks + ); + assert.ok( + parsed.hooks.some((h) => h && h.event === 'SessionStart' && h.command.includes('gsd-check-update.js')), + 'top-level [[hooks]] AoT must contain the GSD SessionStart entry' + ); 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');