Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions bin/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -2977,16 +2977,16 @@ function migrateCodexHooksMapFormat(content) {

/**
* Detect whether the user already uses the namespaced AoT hooks form
* (`[[hooks.<EVENT>]]`) for the given event in the config. When true,
* (`[[hooks.<EVENT>]]`) 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.')
);
}

Expand Down Expand Up @@ -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.<Event>]]` 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}`);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Migrate legacy gsd-update-check entries from prior installs (#1755 followup)
// Remove stale hook blocks that used the inverted filename or wrong path.
Expand Down
6 changes: 3 additions & 3 deletions tests/bug-2760-codex-install-defensive.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -368,7 +368,7 @@ describe('#2760 — hasUserNamespacedAotHooks helper', () => {
'command = "x"',
'',
].join('\n');
assert.equal(hasUserNamespacedAotHooks(content, 'SessionStart'), false);
assert.equal(hasUserNamespacedAotHooks(content), false);
});
});

Expand Down
22 changes: 19 additions & 3 deletions tests/codex-config.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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');
Expand Down