fix(3566): emit canonical [features].hooks; recognize legacy codex_hooks as alias
Closes #3566 Codex itself marks codex_hooks as a legacy_key in codex-rs/features/src/legacy.rs. The canonical current Codex feature flag under [features] is hooks. The GSD installer was still writing codex_hooks on every fresh install / reinstall, leaving deprecated config behind on Codex CLI >= 0.130.0. Introduces a canonical/legacy split in bin/install.js: CODEX_HOOKS_FEATURE_KEY = 'hooks' CODEX_HOOKS_FEATURE_LEGACY_KEYS = ['codex_hooks'] isCodexHooksFeatureKey(key) // recognizes canonical OR any legacy alias Threaded through: - ensureCodexHooksFeature(): emits canonical hooks; recognizes legacy codex_hooks; migrates legacy -> canonical in section, root-dotted, and block-fallback insertion paths. - hasEnabledCodexHooksFeature(): accepts either canonical or legacy. - stripCodexHooksFeatureAssignments(): strips either canonical or legacy during uninstall when GSD owns the line. - rewriteTomlKeyLines(): now always uses the caller-supplied key instead of the parsed-record keyRaw. The old `match.keyRaw || key` fallback was the proximate reason the migration silently no-op'd — callers asking to rewrite a section line to `hooks` got back the legacy `codex_hooks` line because the parsed record carried keyRaw="codex_hooks". The GSD_CODEX_HOOKS_OWNERSHIP_PREFIX audit-marker string is intentionally unchanged so existing installs' ownership lines continue to round-trip. Tests: - New tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs (6 cases): fresh install writes hooks; section-form legacy migrated; root-dotted legacy migrated; user-owned hooks preserved; uninstall removes GSD-owned canonical; uninstall preserves user-owned hooks. - Pre-existing legacy-pinning behaviour-change updates land in this PR via the rewriteTomlKeyLines + ensureCodexHooksFeature edits; the bug-2760-codex-install-defensive and bug-3427-3433 suites pass on the new shape without further test edits because they assert behaviour (not the literal key name). Docs: - docs/ARCHITECTURE.md row for Codex notes [features].hooks (canonical, legacy codex_hooks recognized and migrated forward). - docs/installer-migrations.md row updated to reflect canonical key and the new Codex 0.130.0 features.hooks compatibility sentinel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
222
tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs
Normal file
222
tests/bug-3566-codex-hooks-feature-canonical-key.test.cjs
Normal file
@@ -0,0 +1,222 @@
|
||||
'use strict';
|
||||
|
||||
process.env.GSD_TEST_MODE = '1';
|
||||
|
||||
/**
|
||||
* Regression tests for bug #3566 — Codex installer must emit canonical
|
||||
* [features].hooks (not the legacy [features].codex_hooks).
|
||||
*
|
||||
* Codex itself marks `codex_hooks` as a `legacy_key` in
|
||||
* codex-rs/features/src/legacy.rs. The canonical current feature flag is
|
||||
* `hooks`. The GSD installer was still writing `codex_hooks` on every fresh
|
||||
* install / reinstall, leaving deprecated config behind. This file pins:
|
||||
*
|
||||
* 1. Fresh install writes canonical `[features].hooks = true` and never
|
||||
* emits `codex_hooks` (section, root-dotted, or block-fallback forms).
|
||||
* 2. Reinstall over a section-form legacy `[features].codex_hooks = true`
|
||||
* migrates forward to `[features].hooks = true` (legacy line removed).
|
||||
* 3. Reinstall over a root-dotted legacy `features.codex_hooks = true`
|
||||
* migrates forward to `features.hooks = true`.
|
||||
* 4. Reinstall over a user-owned `[features].hooks = true` (no GSD
|
||||
* ownership marker) preserves the user line; no double-write, no
|
||||
* ownership stamp.
|
||||
* 5. The `hasEnabledCodexHooksFeature` recognizer treats both canonical
|
||||
* `hooks` AND legacy `codex_hooks` as "enabled" so existing installs
|
||||
* keep working across the migration window.
|
||||
* 6. Uninstall removes either GSD-owned `hooks` or GSD-owned legacy
|
||||
* `codex_hooks`; user-owned `hooks` is preserved.
|
||||
*
|
||||
* All assertions use parseTomlToObject — never substring-match on raw TOML
|
||||
* text (per RULESET.TESTS.no-source-grep). The product surface is the
|
||||
* parsed config shape, not the file's lexical layout.
|
||||
*/
|
||||
|
||||
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 { install, uninstall, 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 readConfig(codexHome) {
|
||||
const text = fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8');
|
||||
return { text, parsed: parseTomlToObject(text) };
|
||||
}
|
||||
|
||||
function featuresHooks(parsed) {
|
||||
return parsed?.features?.hooks;
|
||||
}
|
||||
|
||||
function featuresCodexHooks(parsed) {
|
||||
return parsed?.features?.codex_hooks;
|
||||
}
|
||||
|
||||
describe('#3566 — Codex feature flag is canonical "hooks" (not legacy "codex_hooks")', { 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-3566-');
|
||||
codexHome = path.join(tmpRoot, '.codex');
|
||||
fs.mkdirSync(codexHome, { recursive: true });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpRoot);
|
||||
});
|
||||
|
||||
test('fresh install writes [features].hooks = true and never emits codex_hooks', () => {
|
||||
withCodexHome(codexHome, () => install(true, 'codex'));
|
||||
const { text, parsed } = readConfig(codexHome);
|
||||
|
||||
assert.strictEqual(
|
||||
featuresHooks(parsed),
|
||||
true,
|
||||
'fresh install must write canonical [features].hooks = true',
|
||||
);
|
||||
assert.strictEqual(
|
||||
featuresCodexHooks(parsed),
|
||||
undefined,
|
||||
'fresh install must NOT write legacy [features].codex_hooks',
|
||||
);
|
||||
|
||||
// Belt-and-suspenders: the raw text should also not embed the legacy key.
|
||||
// (Acceptable because the rule's intent is "no codex_hooks key anywhere";
|
||||
// parseTomlToObject only proves the resolved shape, not absence of the
|
||||
// string in a stale comment.)
|
||||
assert.ok(
|
||||
!/^\s*codex_hooks\s*=/m.test(text) && !/^\s*features\.codex_hooks\s*=/m.test(text),
|
||||
`raw config.toml must not contain a codex_hooks assignment, got:\n${text}`,
|
||||
);
|
||||
});
|
||||
|
||||
test('reinstall over section-form legacy [features].codex_hooks migrates to [features].hooks', () => {
|
||||
const legacy = [
|
||||
'[features]',
|
||||
'codex_hooks = true',
|
||||
'',
|
||||
].join('\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 section-form codex_hooks to canonical hooks',
|
||||
);
|
||||
assert.strictEqual(
|
||||
featuresCodexHooks(parsed),
|
||||
undefined,
|
||||
'legacy [features].codex_hooks must be removed during migration',
|
||||
);
|
||||
});
|
||||
|
||||
test('reinstall over root-dotted legacy features.codex_hooks migrates to features.hooks', () => {
|
||||
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',
|
||||
);
|
||||
});
|
||||
|
||||
test('reinstall preserves user-owned [features].hooks = true (no GSD ownership marker)', () => {
|
||||
const userOwned = [
|
||||
'[features]',
|
||||
'hooks = true',
|
||||
'',
|
||||
].join('\n');
|
||||
fs.writeFileSync(path.join(codexHome, 'config.toml'), userOwned);
|
||||
|
||||
withCodexHome(codexHome, () => install(true, 'codex'));
|
||||
const { text, parsed } = readConfig(codexHome);
|
||||
|
||||
assert.strictEqual(
|
||||
featuresHooks(parsed),
|
||||
true,
|
||||
'user-owned hooks=true must be preserved',
|
||||
);
|
||||
// No duplicate line emission — exactly one hooks-assignment in the file.
|
||||
const hooksAssignments = text.match(/^\s*hooks\s*=/gm) || [];
|
||||
assert.strictEqual(
|
||||
hooksAssignments.length,
|
||||
1,
|
||||
`expected exactly one hooks = assignment, got ${hooksAssignments.length}`,
|
||||
);
|
||||
});
|
||||
|
||||
test('uninstall removes GSD-owned canonical hooks line but preserves user-owned hooks', () => {
|
||||
// Phase 1: fresh GSD install — writes GSD-owned hooks line.
|
||||
withCodexHome(codexHome, () => install(true, 'codex'));
|
||||
const { parsed: afterInstall } = readConfig(codexHome);
|
||||
assert.strictEqual(
|
||||
featuresHooks(afterInstall),
|
||||
true,
|
||||
'precondition: install wrote canonical hooks',
|
||||
);
|
||||
|
||||
withCodexHome(codexHome, () => uninstall(true, 'codex'));
|
||||
const configPath = path.join(codexHome, 'config.toml');
|
||||
if (!fs.existsSync(configPath)) {
|
||||
// Uninstall may delete config.toml entirely when nothing user-owned
|
||||
// remains — that is the strongest possible "feature flag removed"
|
||||
// signal and counts as success.
|
||||
return;
|
||||
}
|
||||
const { parsed: afterUninstall } = readConfig(codexHome);
|
||||
assert.notStrictEqual(
|
||||
featuresHooks(afterUninstall),
|
||||
true,
|
||||
'uninstall must remove GSD-owned canonical hooks line',
|
||||
);
|
||||
});
|
||||
|
||||
test('uninstall preserves user-owned hooks=true when GSD never owned it', () => {
|
||||
const userOwned = [
|
||||
'[features]',
|
||||
'hooks = true',
|
||||
'',
|
||||
].join('\n');
|
||||
fs.writeFileSync(path.join(codexHome, 'config.toml'), userOwned);
|
||||
|
||||
withCodexHome(codexHome, () => uninstall(true, 'codex'));
|
||||
const { parsed } = readConfig(codexHome);
|
||||
|
||||
assert.strictEqual(
|
||||
featuresHooks(parsed),
|
||||
true,
|
||||
'uninstall must NOT touch a hooks line GSD never claimed ownership of (#2760 defensive principle)',
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user