golem15/msd-core
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(codex): remove duplicate skill copies and consolidate hook payloads

Browse Source
This commit is contained in:
Tom Boucher
2026-05-14 13:17:39 -04:00
parent 625ccf5217
commit 1d6284a718
4 changed files with 351 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files

118
tests/bug-2760-codex-install-defensive.test.cjs
View File

@@ -77,6 +77,27 @@ function writeCodexConfig(codexHome, content) {
fs.writeFileSync(path.join(codexHome, 'config.toml'), content, 'utf8');
}
function readCodexHooksJson(codexHome) {
const hooksPath = path.join(codexHome, 'hooks.json');
if (!fs.existsSync(hooksPath)) return {};
const raw = fs.readFileSync(hooksPath, 'utf8').trim();
if (!raw) return {};
return JSON.parse(raw);
}
function readHooksSessionStartCommands(codexHome) {
const parsed = readCodexHooksJson(codexHome);
const table = (parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks))
? parsed.hooks
: parsed;
const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : [];
return sessionStart.flatMap((entry) =>
(Array.isArray(entry?.hooks) ? entry.hooks : [])
.map((hook) => hook && hook.command)
.filter((cmd) => typeof cmd === 'string')
);
}
describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/reinstall', () => {
let tmpDir;
let codexHome;
@@ -99,37 +120,16 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei
const content = readCodexConfig(codexHome);
const parsed = parseTomlToObject(content);
// hooks must be an object (namespaced), NOT a flat array.
const sessionStartCommands = readHooksSessionStartCommands(codexHome);
const managed = sessionStartCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd));
assert.equal(managed.length, 1, 'hooks.json must contain exactly one managed gsd-check-update command');
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]]'
!parsed.hooks || !Array.isArray(parsed.hooks.SessionStart),
'config.toml should not carry managed SessionStart hooks for GSD'
);
});
test('preserves user [[hooks.SessionStart]] entries and adds GSD nested handler', () => {
test('preserves user [[hooks.SessionStart]] entries and registers managed GSD handler in hooks.json', () => {
// 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 = [
@@ -170,9 +170,10 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei
allCommands.includes('echo second user hook'),
'second user hook preserved: ' + JSON.stringify(allCommands)
);
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
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)
hooksJsonCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)),
'GSD handler must appear in hooks.json SessionStart entries: ' + JSON.stringify(hooksJsonCommands)
);
assert.ok(!Array.isArray(parsed.hooks), 'no flat [[hooks]] entries');
});
@@ -197,16 +198,9 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei
// 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));
// Only one GSD hook entry must exist (no duplication) in hooks.json.
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
const gsdHandlers = hooksJsonCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd));
assert.strictEqual(gsdHandlers.length, 1, 'exactly one managed handler after upgrade');
});
@@ -228,20 +222,9 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei
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.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'
);
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
const gsdHandlers = hooksJsonCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd));
assert.strictEqual(gsdHandlers.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', () => {
@@ -250,12 +233,9 @@ describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/rei
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');
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
const gsdHandlers = hooksJsonCommands.filter((cmd) => /gsd-check-update\.js/.test(cmd));
assert.strictEqual(gsdHandlers.length, 1, 'exactly one managed SessionStart handler after double install');
});
});
@@ -686,10 +666,11 @@ describe('#2760 CR4 finding 2 — Legacy flat [[hooks]] block migrates to namesp
allSessionStartCommands.includes('echo user hook'),
'user [[hooks.SessionStart]] entry preserved: ' + JSON.stringify(allSessionStartCommands)
);
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
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(allSessionStartCommands)
hooksJsonCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)),
'GSD entry must appear in hooks.json SessionStart entries: '
+ JSON.stringify(hooksJsonCommands)
);
// The legacy top-level [[hooks]] AoT must NOT coexist with the namespaced
@@ -701,9 +682,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 = allSessionStartCommands.filter(
(cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)
);
const gsdEntries = hooksJsonCommands.filter((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd));
assert.equal(gsdEntries.length, 1,
'exactly one gsd-check-update entry after migration, got: ' + gsdEntries.length);
});
@@ -809,7 +788,7 @@ describe('#2760 CR4 finding 1 — atomicWriteFileSync failure aborts install (po
let isHookWrite = false;
try {
const data = fs.readFileSync(src, 'utf8');
isHookWrite = /gsd-check-update\.js/.test(data);
isHookWrite = /GSD codex_hooks ownership/.test(data);
} catch (_) { /* ignore */ }
if (isHookWrite) {
throw new Error('simulated rename failure');
@@ -1083,10 +1062,11 @@ describe('#2760 CR5 finding 3 — migration emits namespaced AoT (no flat/namesp
JSON.stringify(ssCommands)
);
// GSD's managed gsd-check-update entry also lives in the namespaced array.
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
assert.ok(
ssCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)),
'managed gsd-check-update entry must appear in hooks.SessionStart array: ' +
JSON.stringify(ssCommands)
hooksJsonCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)),
'managed gsd-check-update entry must appear in hooks.json SessionStart entries: ' +
JSON.stringify(hooksJsonCommands)
);
// No flat top-level [[hooks]] AoT may remain.

32
tests/bug-3357-codex-legacy-hooks-json-migration.test.cjs
View File

@@ -1,9 +1,10 @@
/**
* Regression test for bug #3357.
*
* Older Codex installs used hooks.json for SessionStart hooks. Current Codex
* installs write config.toml hooks. Reinstalling must remove only GSD-managed
* legacy hooks.json entries so users do not end up with duplicate GSD hooks.
* Older Codex installs carried legacy GSD SessionStart commands in hooks.json.
* 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.
*/
'use strict';
@@ -14,11 +15,14 @@ const { describe, test, beforeEach, afterEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const { execFileSync } = require('node:child_process');
const installModule = require('../bin/install.js');
const { readInstallState } = require('../get-shit-done/bin/lib/installer-migrations.cjs');
const { install, parseTomlToObject } = 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');
function withCodexHome(codexHome, fn) {
const previousCodexHome = process.env.CODEX_HOME;
@@ -63,6 +67,9 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc
let codexHome;
beforeEach(() => {
if (!fs.existsSync(HOOKS_DIST) || fs.readdirSync(HOOKS_DIST).length === 0) {
execFileSync(process.execPath, [BUILD_HOOKS_SCRIPT], { stdio: 'pipe' });
}
tmpRoot = createTempDir('gsd-3357-');
codexHome = path.join(tmpRoot, '.codex');
fs.mkdirSync(codexHome, { recursive: true });
@@ -73,7 +80,7 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc
cleanup(tmpRoot);
});
test('removes hooks.json when it only contained the legacy GSD SessionStart hook', () => {
test('rewrites hooks.json to one managed SessionStart hook when file only had legacy managed entry', () => {
fs.writeFileSync(
path.join(codexHome, 'hooks.json'),
JSON.stringify({ SessionStart: [legacyGsdHook(codexHome)] }, null, 2),
@@ -81,8 +88,11 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc
withCodexHome(codexHome, () => install(true, 'codex'));
assert.equal(fs.existsSync(path.join(codexHome, 'hooks.json')), false);
assert.equal(tomlGsdHookCount(codexHome), 1);
const hooksJson = JSON.parse(fs.readFileSync(path.join(codexHome, 'hooks.json'), 'utf8'));
const commands = hooksJson.SessionStart.flatMap((entry) => entry.hooks).map((hook) => hook.command);
const managed = commands.filter((cmd) => typeof cmd === 'string' && cmd.includes('gsd-check-update.js'));
assert.equal(managed.length, 1);
assert.equal(tomlGsdHookCount(codexHome), 0);
});
test('preserves user hooks.json entries while removing the legacy GSD hook', () => {
@@ -101,11 +111,11 @@ describe('#3357 — Codex install removes legacy GSD hooks.json entries', { conc
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.deepEqual(commands, [
'node "/Users/example/bin/user-hook.js"',
'node "/Users/example/bin/gsd-check-update.js"',
]);
assert.equal(tomlGsdHookCount(codexHome), 1);
const managed = commands.filter((cmd) => typeof cmd === 'string' && cmd.includes('gsd-check-update.js'));
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);
assert.equal(managed.length, 2);
assert.equal(tomlGsdHookCount(codexHome), 0);
});
test('restores migrated hooks.json and install state when later Codex validation fails', () => {

123
tests/bug-3427-3433-codex-install-shape.test.cjs Normal file
View File

@@ -0,0 +1,123 @@
'use strict';
process.env.GSD_TEST_MODE = '1';
const { describe, test, beforeEach, afterEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const crypto = require('node:crypto');
const { execFileSync } = require('node:child_process');
const { install, parseTomlToObject } = require('../bin/install.js');
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');
function withCodexHome(codexHome, fn) {
const prev = process.env.CODEX_HOME;
process.env.CODEX_HOME = codexHome;
try {
return fn();
} finally {
if (prev == null) delete process.env.CODEX_HOME;
else process.env.CODEX_HOME = prev;
}
}
function extractSessionStartCommandsFromHooksJson(value) {
if (!value || typeof value !== 'object' || Array.isArray(value)) return [];
const table = (value.hooks && typeof value.hooks === 'object' && !Array.isArray(value.hooks))
? value.hooks
: value;
const sessionStart = Array.isArray(table.SessionStart) ? table.SessionStart : [];
return sessionStart.flatMap((entry) => {
const hooks = entry && Array.isArray(entry.hooks) ? entry.hooks : [];
return hooks.map((h) => h && h.command).filter((cmd) => typeof cmd === 'string');
});
}
describe('#3427 + #3433 — Codex installer avoids duplicate skills and mixed hook representation', { concurrency: false }, () => {
let tmpRoot;
let codexHome;
beforeEach(() => {
if (!fs.existsSync(HOOKS_DIST) || fs.readdirSync(HOOKS_DIST).length === 0) {
execFileSync(process.execPath, [BUILD_HOOKS_SCRIPT], { stdio: 'pipe' });
}
tmpRoot = createTempDir('gsd-3427-3433-');
codexHome = path.join(tmpRoot, '.codex');
fs.mkdirSync(codexHome, { recursive: true });
});
afterEach(() => {
cleanup(tmpRoot);
});
test('removes legacy gsd-* copies from ~/.codex/skills and does not regenerate them', () => {
const legacySkillBody = '# old managed\n';
fs.mkdirSync(path.join(codexHome, 'skills', 'gsd-help'), { recursive: true });
fs.writeFileSync(path.join(codexHome, 'skills', 'gsd-help', 'SKILL.md'), legacySkillBody);
const legacyHash = crypto.createHash('sha256').update(legacySkillBody).digest('hex');
fs.writeFileSync(path.join(codexHome, 'gsd-file-manifest.json'), JSON.stringify({
version: 1,
files: {
'skills/gsd-help/SKILL.md': legacyHash,
},
}, null, 2));
fs.mkdirSync(path.join(codexHome, 'skills', 'custom-user-skill'), { recursive: true });
fs.writeFileSync(path.join(codexHome, 'skills', 'custom-user-skill', 'SKILL.md'), '# user skill\n');
withCodexHome(codexHome, () => install(true, 'codex'));
const skillsDir = path.join(codexHome, 'skills');
const entries = fs.existsSync(skillsDir)
? fs.readdirSync(skillsDir, { withFileTypes: true }).filter((e) => e.isDirectory()).map((e) => e.name)
: [];
assert.equal(entries.some((name) => name.startsWith('gsd-')), false);
assert.equal(entries.includes('custom-user-skill'), true);
});
test('stores managed SessionStart update hook in hooks.json and removes inline gsd hook from config.toml', () => {
const configToml = [
'[features]',
'codex_hooks = true',
'',
'[[hooks.SessionStart]]',
'[[hooks.SessionStart.hooks]]',
'type = "command"',
'command = "node /tmp/legacy/.codex/hooks/gsd-check-update.js"',
'',
].join('\n');
fs.writeFileSync(path.join(codexHome, 'config.toml'), configToml);
fs.writeFileSync(path.join(codexHome, 'hooks.json'), JSON.stringify({
SessionStart: [
{
hooks: [
{ type: 'command', command: 'node "/Users/example/bin/user-hook.js"' },
],
},
],
}, null, 2));
withCodexHome(codexHome, () => install(true, 'codex'));
const parsedToml = parseTomlToObject(fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8'));
const tomlSessionStart = parsedToml.hooks?.SessionStart ?? [];
const tomlCommands = tomlSessionStart.flatMap((entry) =>
(Array.isArray(entry?.hooks) ? entry.hooks : []).map((hook) => hook.command).filter((cmd) => typeof cmd === 'string')
);
assert.equal(tomlCommands.some((cmd) => cmd.includes('gsd-check-update.js')), false);
const hooksJson = JSON.parse(fs.readFileSync(path.join(codexHome, 'hooks.json'), 'utf8'));
const sessionStartCommands = extractSessionStartCommandsFromHooksJson(hooksJson);
const gsdCommands = sessionStartCommands.filter((cmd) => cmd.includes('gsd-check-update.js'));
assert.equal(gsdCommands.length, 1);
assert.equal(sessionStartCommands.includes('node "/Users/example/bin/user-hook.js"'), true);
});
});