From 1746f89ac73208dc8311723573b5b5ad7c91a5a0 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 14:22:32 -0400 Subject: [PATCH] fix(3566): narrow scope; update codex-config tests for canonical hooks key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../3566-codex-hooks-canonical-feature-key.md | 2 +- bin/install.js | 16 +++---- ...codex-hooks-feature-canonical-key.test.cjs | 25 ++++------ tests/codex-config.test.cjs | 48 +++++++++---------- 4 files changed, 42 insertions(+), 49 deletions(-) diff --git a/.changeset/3566-codex-hooks-canonical-feature-key.md b/.changeset/3566-codex-hooks-canonical-feature-key.md index 2db8d5a2b..1d15dcd53 100644 --- a/.changeset/3566-codex-hooks-canonical-feature-key.md +++ b/.changeset/3566-codex-hooks-canonical-feature-key.md @@ -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. diff --git a/bin/install.js b/bin/install.js index ea207e657..2e920fcb4 100755 --- a/bin/install.js +++ b/bin/install.js @@ -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; } diff --git a/tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs b/tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs index 8e75874bb..dc5bd8c5f 100644 --- a/tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs +++ b/tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs @@ -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', ); }); diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 62c6c4d11..b53b5636c 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -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.] 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.] 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); }); });