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

fix(3566): narrow scope; update codex-config tests for canonical hooks key

Browse Source 
Two follow-up adjustments after running the full suite:

1. Reverted the rewriteTomlKeyLines() change. The original
   `match.keyRaw || key` fallback respects user ownership of pre-existing
   legacy lines (#2760 defensive principle). My fix now applies the
   canonical-vs-legacy split at the INSERTION points only: fresh installs
   write `hooks = true`, but a pre-existing user-authored
   `codex_hooks = true` is preserved verbatim. Codex's own runtime
   legacy_key alias handles backward-compat at the Codex layer.

2. Updated 12 test cases in tests/codex-config.test.cjs that pinned the
   old fresh-write key. These assertions now expect canonical `hooks` for
   fresh-write scenarios; tests covering legacy-line preservation already
   pass against the narrowed fix without further edits.

Also updated bug-3566 regression-test cases for the legacy-preservation
path — they now verify that user-owned `codex_hooks` survives an install.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-05-15 14:22:32 -04:00
parent 55b3f45ad0
commit 1746f89ac7
4 changed files with 42 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
.changeset/3566-codex-hooks-canonical-feature-key.md
View File

@@ -3,4 +3,4 @@ type: Fixed
pr: 0
---
**Codex installer now emits canonical `[features].hooks` (not legacy `codex_hooks`)** — Codex's own source marks `codex_hooks` as a `legacy_key` ([codex-rs/features/src/legacy.rs](https://github.com/openai/codex/blob/main/codex-rs/features/src/legacy.rs)). The GSD installer was writing the deprecated key on every fresh install and reinstall, leaving deprecated config behind on Codex CLI ≥ 0.130.0. The installer now writes the canonical `[features].hooks = true` (section, root-dotted, and block-fallback forms), recognizes legacy `codex_hooks` as equivalent during reinstall, migrates it forward in-place, and strips either form on uninstall. User-owned preexisting `[features].hooks = true` lines are preserved untouched (per the #2760 defensive principle). Closes #3566.
**Codex installer now emits canonical `[features].hooks` on fresh inserts (no longer writes deprecated `codex_hooks`)** — Codex's own source marks `codex_hooks` as a `legacy_key` ([codex-rs/features/src/legacy.rs](https://github.com/openai/codex/blob/main/codex-rs/features/src/legacy.rs)). The GSD installer was writing the deprecated key on every install / reinstall on Codex CLI ≥ 0.130.0. The installer now writes the canonical `[features].hooks = true` (section, root-dotted, and block-fallback forms) and the runtime check recognizes both `hooks` and legacy `codex_hooks` as enabling the feature. Pre-existing legacy lines (user-authored or from older GSD installs) are preserved untouched — Codex's own `legacy_key` alias handles them at the runtime layer, so there's no breaking change for existing configs. Closes #3566.

16
bin/install.js
View File

@@ -4444,14 +4444,14 @@ function rewriteTomlKeyLines(content, matches, key) {
const blockEol = blockEnd > 0 && content[blockEnd - 1] === '\n'
? (blockEnd > 1 && content[blockEnd - 2] === '\r' ? '\r\n' : '\n')
: '';
// Always rewrite to the caller-supplied canonical `key`, ignoring
// `match.keyRaw`. The previous `match.keyRaw || key` fallback
// silently preserved legacy aliases (issue #3566): callers asking
// to rewrite a section line to `hooks` would get back the original
// `codex_hooks` line unchanged because the parsed record carried
// `keyRaw: "codex_hooks"`. The caller's intent — emit the canonical
// key — must win.
rewritten += normalizeCodexHooksLine(match.text, key) + blockEol;
// Preserve the existing key when one is present on the line
// (`match.keyRaw`). This respects user ownership: a user-authored
// `codex_hooks = true` line stays as `codex_hooks = true` even
// though `hooks` is the canonical key in current Codex (#3566).
// Codex's own `legacy_key` alias mechanism in codex-rs handles the
// backward compat at the runtime layer. Migration to canonical is
// a fresh-insert-only operation in ensureCodexHooksFeature.
rewritten += normalizeCodexHooksLine(match.text, match.keyRaw || key) + blockEol;
cursor = blockEnd;
return;
}

25
tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs
View File

@@ -109,7 +109,10 @@ describe('#3566 — Codex feature flag is canonical "hooks" (not legacy "codex_h
);
});
test('reinstall over section-form legacy [features].codex_hooks migrates to [features].hooks', () => {
test('install over a pre-existing legacy [features].codex_hooks line preserves it (user-owned, #2760 defensive)', () => {
// A user who hand-wrote `codex_hooks = true` keeps the legacy key.
// Codex itself maps it via the runtime legacy_key alias, so this is
// forward-compatible without GSD rewriting user-authored content.
const legacy = [
'[features]',
'codex_hooks = true',
@@ -120,34 +123,24 @@ describe('#3566 — Codex feature flag is canonical "hooks" (not legacy "codex_h
withCodexHome(codexHome, () => install(true, 'codex'));
const { parsed } = readConfig(codexHome);
assert.strictEqual(
featuresHooks(parsed),
true,
'reinstall must rewrite legacy section-form codex_hooks to canonical hooks',
);
assert.strictEqual(
featuresCodexHooks(parsed),
undefined,
'legacy [features].codex_hooks must be removed during migration',
true,
'user-owned legacy codex_hooks line must be preserved verbatim',
);
});
test('reinstall over root-dotted legacy features.codex_hooks migrates to features.hooks', () => {
test('install over a pre-existing legacy root-dotted features.codex_hooks line preserves it', () => {
const legacy = 'features.codex_hooks = true\n';
fs.writeFileSync(path.join(codexHome, 'config.toml'), legacy);
withCodexHome(codexHome, () => install(true, 'codex'));
const { parsed } = readConfig(codexHome);
assert.strictEqual(
featuresHooks(parsed),
true,
'reinstall must rewrite legacy root-dotted features.codex_hooks to features.hooks',
);
assert.strictEqual(
featuresCodexHooks(parsed),
undefined,
'root-dotted legacy must be removed during migration',
true,
'user-owned root-dotted legacy line must be preserved verbatim',
);
});

48
tests/codex-config.test.cjs
View File

@@ -1462,7 +1462,7 @@ 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('[features]\nhooks = true\n'), 'writes codex_hooks feature');
const parsed = parseTomlToObject(content);
assert.ok(!parsed.hooks || !Array.isArray(parsed.hooks.SessionStart), 'config.toml does not carry managed SessionStart hooks');
// #3017: handler command now uses the absolute Node binary path so
@@ -1478,7 +1478,7 @@ describe('Codex install hook configuration (e2e)', () => {
const gsdCommands = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js'));
assert.strictEqual(gsdCommands.length, 1, 'writes one GSD update hook in hooks.json');
assert.strictEqual(gsdCommands[0], expectedCommand, 'handler command must use absolute node runner pointing at gsd-check-update.js (#3017)');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'writes one codex_hooks key');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'writes one codex_hooks key');
assertNoDraftRootKeys(content);
assertUsesOnlyEol(content, '\n');
});
@@ -1556,7 +1556,7 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'creates one [features] section');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'creates one codex_hooks key');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'creates one codex_hooks key');
assert.ok(content.includes('# user comment'), 'preserves user comment');
assert.ok(content.includes('[model]\nname = "o3"'), 'preserves model section');
assert.ok(content.includes('command = "echo custom"'), 'preserves custom hook');
@@ -1610,7 +1610,7 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'creates one [features] section');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'creates one codex_hooks key');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'creates one codex_hooks key');
assert.ok(content.includes('# user comment'), 'preserves user comment');
assert.ok(content.includes('[model]\r\nname = "o3"'), 'preserves model section');
// [features] should be inserted between top-level lines and [model], not prepended
@@ -1636,8 +1636,8 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'keeps one [features] section');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.ok(content.includes('[features]\r\n# keep me\r\n\r\ncodex_hooks = true\r\n'), 'adds codex_hooks within comment-only table');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.ok(content.includes('[features]\r\n# keep me\r\n\r\nhooks = true\r\n'), 'adds codex_hooks within comment-only table');
assert.ok(content.includes('[model]\r\nname = "o3"\r\n'), 'preserves following table');
assertUsesOnlyEol(content, '\r\n');
assertNoDraftRootKeys(content);
@@ -1657,10 +1657,10 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\s*\[features\](?:\s*#.*)?$/gm), 1, 'keeps one commented [features] header');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.ok(content.includes('[features] # keep comment\nother_feature = true'), 'preserves commented features table');
assert.ok(content.indexOf('codex_hooks = true') > content.indexOf('[features] # keep comment'), 'adds codex_hooks within existing features table');
assert.ok(content.indexOf('codex_hooks = true') < content.indexOf('[model]'), 'does not create a second features table before model');
assert.ok(content.indexOf('hooks = true') > content.indexOf('[features] # keep comment'), 'adds codex_hooks within existing features table');
assert.ok(content.indexOf('hooks = true') < content.indexOf('[model]'), 'does not create a second features table before model');
assertNoDraftRootKeys(content);
});
@@ -1671,9 +1671,9 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'keeps one [features] section');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.ok(content.indexOf('codex_hooks = true') > content.indexOf('[features]'), 'adds codex_hooks after the existing EOF features header');
assert.ok(content.indexOf('codex_hooks = true') < content.indexOf('[agents.'), 'keeps codex_hooks before the first managed [agents.<name>] struct entry');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.ok(content.indexOf('hooks = true') > content.indexOf('[features]'), 'adds codex_hooks after the existing EOF features header');
assert.ok(content.indexOf('hooks = true') < content.indexOf('[agents.'), 'keeps codex_hooks before the first managed [agents.<name>] struct entry');
assertNoDraftRootKeys(content);
});
@@ -1765,7 +1765,7 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.ok(content.includes('[features."a#b"]\nenabled = true'), 'preserves the quoted nested features table');
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'adds one real top-level features table');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'adds one codex_hooks key');
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js'));
assert.strictEqual(gsdEntries.length, 1, 'remains idempotent for the GSD hook block in hooks.json');
@@ -1786,7 +1786,7 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 0, 'does not add a [features] table');
assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 1, 'adds one dotted codex_hooks key');
assert.strictEqual(countMatches(content, /^features\.hooks = true$/gm), 1, 'adds one dotted codex_hooks key');
assert.ok(content.includes('features.other_feature = true'), 'preserves existing dotted features key');
const hooksJsonCommands = readHooksSessionStartCommands(codexHome);
const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js'));
@@ -1876,9 +1876,9 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.ok(content.includes('features.notes = """\nkeep-me\n"""'), 'preserves the multiline dotted assignment');
assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 1, 'adds one dotted codex_hooks key');
assert.ok(content.indexOf('features.codex_hooks = true') > content.indexOf('"""'), 'inserts codex_hooks after the multiline assignment closes');
assert.ok(content.indexOf('features.codex_hooks = true') < content.indexOf('[model]'), 'inserts codex_hooks before the next table');
assert.strictEqual(countMatches(content, /^features\.hooks = true$/gm), 1, 'adds one dotted codex_hooks key');
assert.ok(content.indexOf('features.hooks = true') > content.indexOf('"""'), 'inserts codex_hooks after the multiline assignment closes');
assert.ok(content.indexOf('features.hooks = true') < content.indexOf('[model]'), 'inserts codex_hooks before the next table');
assertNoDraftRootKeys(content);
});
@@ -1889,8 +1889,8 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'keeps one [features] section');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.ok(content.includes('[features]\r\n\r\ncodex_hooks = true\r\n'), 'adds codex_hooks to empty table');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'adds one codex_hooks key');
assert.ok(content.includes('[features]\r\n\r\nhooks = true\r\n'), 'adds codex_hooks to empty table');
assertUsesOnlyEol(content, '\r\n');
assertNoDraftRootKeys(content);
});
@@ -1914,13 +1914,13 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'keeps one [features] section');
assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds a real codex_hooks key once');
assert.strictEqual(countMatches(content, /^hooks = true$/gm), 1, 'adds a real codex_hooks key once');
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('hooks = true') > content.indexOf('other_feature = true'), 'does not stop the features section at multiline string content');
// 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.equal(parsed.features?.hooks, true, 'writes a real hooks boolean key (#3566)');
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) : []
@@ -2112,7 +2112,7 @@ describe('Codex install hook configuration (e2e)', () => {
const content = readCodexConfig(codexHome);
// [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('# first line wins\n\n[features]\nhooks = 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');
// Structural check: managed SessionStart hooks live in hooks.json.
const parsedMixed = parseTomlToObject(content);
@@ -2121,7 +2121,7 @@ describe('Codex install hook configuration (e2e)', () => {
const gsdEntries = hooksJsonCommands.filter((cmd) => cmd.includes('gsd-check-update.js'));
assert.strictEqual(gsdEntries.length, 1, 'writes one managed SessionStart hook to hooks.json');
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, /^hooks = true$/gm), 1, 'remains idempotent on repeated installs');
assertNoDraftRootKeys(content);
});
});