From e14ef535aaca1e6b52de9c600f8f579c77c216e0 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 9 May 2026 07:42:22 -0400 Subject: [PATCH] fix(install): allow codex hooks.state. as regular table (#3285) (#3289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: codex hooks.state. tables must validate as regular tables (#3285 RED) Drive validateCodexConfigSchema with a fixture containing both [hooks.state] and [[hooks.SessionStart]] entries. Expect the state tables to pass as regular tables. Currently fails — validator over-classifies every hooks.* path as AoT. Co-Authored-By: Claude Sonnet 4.6 * fix(install): treat codex hooks.state as regular table not AoT (#3285) validateCodexConfigSchema was over-classifying every hooks.* section header as an event-handler array-of-tables, rejecting the hooks.state.* namespace that Codex CLI 0.130.0+ uses for per-hook trust persistence. Fix: 1. Section-header check: carve out `hooks.state` and `hooks.state.*` from the AoT-required rule — only paths that are neither of those still require double-bracket form. 2. Parsed-object check: skip the `state` key when iterating Object.entries (parsed.hooks) so the "must be array" guard does not fire for the trust namespace object. All other hooks. validation (SessionStart AoT, handler-field placement) is unchanged. Co-Authored-By: Claude Sonnet 4.6 * changeset: pr=3289 for #3285 Co-Authored-By: Claude Sonnet 4.6 * fix(install): codex hooks.state must be regular-table, reject AoT/scalar (CR finding 1) - migrateCodexHooksMapFormat: exclude hooks.state and hooks.state.* from legacy-map detection so [hooks.state] is never promoted to [[hooks.state]] AoT - validateCodexConfigSchema: reject [[hooks.state]] / [[hooks.state.*]] AoT at section level; reject Array/scalar values at parsed-object level - Accept only plain-object shape for hooks.state and hooks.state.* (Codex CLI 0.130.0+ trust-persistence namespace) Co-Authored-By: Claude Sonnet 4.6 * test: assert codex hooks.state trust entry preserved with original values (CR finding 2) Strengthen post-install preservation assertion to verify the actual trust entry key and its enabled/trusted_hash values survive — not just that hooks.state is an object. Add two validator-reject tests for [[hooks.state]] and [[hooks.state.foo]] AoT forms. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .changeset/mellow-lynx-forage.md | 5 + bin/install.js | 38 ++- ...ug-3285-codex-hooks-state-allowed.test.cjs | 282 ++++++++++++++++++ 3 files changed, 323 insertions(+), 2 deletions(-) create mode 100644 .changeset/mellow-lynx-forage.md create mode 100644 tests/bug-3285-codex-hooks-state-allowed.test.cjs diff --git a/.changeset/mellow-lynx-forage.md b/.changeset/mellow-lynx-forage.md new file mode 100644 index 000000000..df15f83fd --- /dev/null +++ b/.changeset/mellow-lynx-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3289 +--- +**`get-shit-done-cc --codex` no longer rejects valid Codex `hooks.state` trust-persistence entries** — the schema validator was over-classifying every `hooks.*` table as an event-handler array-of-tables, breaking installs against Codex CLI 0.130.0+ where `hooks.state./...` stores per-hook trust state. Regular-table shape is now accepted for `hooks.state.*` while `hooks.` still requires AoT. diff --git a/bin/install.js b/bin/install.js index 5830c4333..1fa5a91ae 100755 --- a/bin/install.js +++ b/bin/install.js @@ -3336,10 +3336,13 @@ function migrateCodexHooksMapFormat(content) { // Use section.segments (parsed key count) rather than section.path.startsWith() so that // nested handler tables like [hooks.SessionStart.hooks] (3 segments) are not mistakenly // included and re-emitted as an event named "SessionStart.hooks". + // Exclude hooks.state and hooks.state.* — these are Codex's persistent hook-trust + // namespace (Codex CLI 0.130.0+) and use regular-table shape, never AoT. const legacyMapSections = sections.filter( (section) => !section.array && ( section.path === 'hooks' || - (section.path.startsWith('hooks.') && section.segments.length === 2) + (section.path.startsWith('hooks.') && section.segments.length === 2 && + section.path !== 'hooks.state' && !section.path.startsWith('hooks.state.')) ) ); @@ -3977,7 +3980,20 @@ function validateCodexConfigSchema(content) { }; } - if (!section.array && section.path.startsWith('hooks.')) { + // hooks.state.* is Codex's persistent hook-trust namespace (added in + // Codex CLI 0.130.0). It uses regular-table shape, NOT array-of-tables. + // [[hooks.state]] or [[hooks.state.]] (AoT) is invalid; reject it. + if (section.array && (section.path === 'hooks.state' || section.path.startsWith('hooks.state.'))) { + return { + ok: false, + reason: `[[${section.path}]] is invalid; hooks.state namespace must use regular tables`, + }; + } + + // All other hooks.* paths (event handlers like hooks.SessionStart) require + // AoT shape — bare [hooks.] (single-bracket) is invalid. + if (!section.array && section.path.startsWith('hooks.') && + section.path !== 'hooks.state' && !section.path.startsWith('hooks.state.')) { return { ok: false, reason: `bare [${section.path}] table is invalid in current Codex schema (expected [[${section.path}]] array-of-tables)`, @@ -3997,6 +4013,24 @@ function validateCodexConfigSchema(content) { } if (typeof parsed.hooks === 'object' && parsed.hooks !== null) { for (const [event, value] of Object.entries(parsed.hooks)) { + // hooks.state is Codex's persistent hook-trust namespace — a regular + // object (table), not an array of event-handler tables. + // Reject AoT shape (Array) and scalar forms; only plain objects are valid. + if (event === 'state') { + if (Array.isArray(value)) { + return { + ok: false, + reason: `hooks.state must be a regular table/object, got array-of-tables`, + }; + } + if (typeof value !== 'object' || value === null) { + return { + ok: false, + reason: `hooks.state must be a regular table/object, got ${typeof value}`, + }; + } + continue; + } // Skip the nested .hooks sub-array — it lives under hooks.[n].hooks // and is validated separately below. if (!Array.isArray(value)) { diff --git a/tests/bug-3285-codex-hooks-state-allowed.test.cjs b/tests/bug-3285-codex-hooks-state-allowed.test.cjs new file mode 100644 index 000000000..abca2a395 --- /dev/null +++ b/tests/bug-3285-codex-hooks-state-allowed.test.cjs @@ -0,0 +1,282 @@ +/** + * Regression: issue #3285 — Codex install fails when config.toml contains + * hooks.state entries. + * + * Root cause: validateCodexConfigSchema walks every `hooks.*` table section + * and asserts array-of-tables (AoT) shape, without distinguishing the + * `hooks.state.*` namespace (Codex-managed per-hook trust persistence, a + * regular table) from `hooks.` (event handlers like SessionStart, + * which DO require AoT shape via [[hooks.SessionStart]]). + * + * Fix: add a carve-out so that any table whose path starts with `hooks.state` + * is validated as a regular table (not AoT). All `hooks.` paths still + * require AoT. + */ + +// GSD_TEST_MODE must be set before require('../bin/install.js') so the module +// skips the main CLI entry point and exports its internals. +const previousGsdTestMode = process.env.GSD_TEST_MODE; +process.env.GSD_TEST_MODE = '1'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const { execFileSync } = require('child_process'); + +const { validateCodexConfigSchema, install } = require('../bin/install.js'); +const installModule = require('../bin/install.js'); + +if (previousGsdTestMode === undefined) { + delete process.env.GSD_TEST_MODE; +} else { + process.env.GSD_TEST_MODE = previousGsdTestMode; +} + +// Ensure hooks/dist/ is populated — mirrors the pattern used by codex-config.test.cjs. +const { before, beforeEach, afterEach } = require('node:test'); +const HOOKS_DIST = path.join(__dirname, '..', 'hooks', 'dist'); +const BUILD_HOOKS_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); +before(() => { + if (!fs.existsSync(HOOKS_DIST) || fs.readdirSync(HOOKS_DIST).length === 0) { + execFileSync(process.execPath, [BUILD_HOOKS_SCRIPT], { encoding: 'utf-8', stdio: 'pipe' }); + } +}); + +// --------------------------------------------------------------------------- +// Validator unit tests (no install, just validateCodexConfigSchema) +// --------------------------------------------------------------------------- + +describe('#3285 — validateCodexConfigSchema: hooks.state is a regular table (not AoT)', () => { + test('bare [hooks.state] table header passes validation', () => { + const content = [ + '[hooks.state]', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, true, + 'bare [hooks.state] must be allowed (regular-table namespace): ' + result.reason); + }); + + test('bare [hooks.state.] table header passes validation', () => { + // Mirrors the exact shape Codex CLI 0.130.0+ writes for per-hook trust entries. + // The key contains slashes and colons — must be quoted in TOML. + const content = [ + '[hooks.state]', + '', + "[hooks.state.'/home/user/.codex/hooks.json:pre_tool_use:0:0']", + 'enabled = true', + 'trusted_hash = "sha256:abc123"', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, true, + 'bare [hooks.state.] with trust fields must be allowed: ' + result.reason); + }); + + test('hooks.state alongside [[hooks.SessionStart]] AoT both pass', () => { + // The real-world fixture: user has both Codex trust state AND GSD-managed + // event hooks in the same config.toml. + const content = [ + '[hooks.state]', + '', + "[hooks.state.'/home/user/.codex/hooks.json:pre_tool_use:0:0']", + 'enabled = true', + 'trusted_hash = "sha256:abc123"', + '', + '[[hooks.SessionStart]]', + '', + '[[hooks.SessionStart.hooks]]', + 'type = "command"', + 'command = "/usr/local/bin/gsd-check-update"', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, true, + 'mixed hooks.state (regular table) + [[hooks.SessionStart]] (AoT) must pass: ' + result.reason); + }); + + test('[[hooks.SessionStart]] AoT still requires array-of-tables shape', () => { + // Regression guard: the fix must NOT relax AoT requirements for event hooks. + // [hooks.SessionStart] (single-bracket) must still fail. + const content = [ + '[hooks.SessionStart]', + 'type = "command"', + 'command = "/some/command"', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, false, + '[hooks.SessionStart] bare table (not AoT) must still be rejected'); + assert.ok( + result.reason.includes('hooks.SessionStart'), + 'rejection reason must mention hooks.SessionStart, got: ' + result.reason + ); + }); + + test('hooks.state object in parsed structure does not trigger non-array rejection', () => { + // The parsed-object check loops over Object.entries(parsed.hooks) and + // asserts !Array.isArray(value) → error. hooks.state is an object, not + // an array. The fix must skip hooks.state in that loop too. + const content = [ + '[hooks.state]', + '', + "[hooks.state.'some-key']", + 'enabled = true', + 'trusted_hash = "sha256:deadbeef"', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, true, + 'parsed hooks.state object must not trigger "hooks.state must be an array" rejection: ' + result.reason); + }); + + test('multiple hooks.state sub-keys all pass validation', () => { + const content = [ + '[hooks.state]', + '', + "[hooks.state.'/project/a/.codex/hooks.json:pre_tool_use:0:0']", + 'enabled = true', + 'trusted_hash = "sha256:aaa"', + '', + "[hooks.state.'/project/b/.codex/hooks.json:pre_tool_use:0:0']", + 'enabled = false', + 'trusted_hash = "sha256:bbb"', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, true, + 'multiple hooks.state sub-keys must all pass: ' + result.reason); + }); + + test('[[hooks.state]] AoT form is rejected', () => { + // hooks.state must be a regular table — array-of-tables shape is invalid. + const content = [ + '[[hooks.state]]', + 'enabled = true', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, false, + '[[hooks.state]] (AoT) must be rejected'); + assert.ok( + result.reason.includes('hooks.state'), + 'rejection reason must mention hooks.state, got: ' + result.reason + ); + }); + + test('[[hooks.state.foo]] AoT sub-key form is rejected', () => { + // hooks.state.* sub-keys must be regular tables — AoT sub-key shape is invalid. + const content = [ + '[[hooks.state.foo]]', + 'enabled = true', + '', + ].join('\n'); + const result = validateCodexConfigSchema(content); + assert.strictEqual(result.ok, false, + '[[hooks.state.foo]] (AoT sub-key) must be rejected'); + assert.ok( + result.reason.includes('hooks.state'), + 'rejection reason must mention hooks.state, got: ' + result.reason + ); + }); +}); + +// --------------------------------------------------------------------------- +// Full install integration test +// --------------------------------------------------------------------------- + +describe('#3285 — install succeeds when config.toml contains hooks.state entries', { concurrency: false }, () => { + let tmpDir; + let codexHome; + + function writeCodexConfig(content) { + fs.mkdirSync(codexHome, { recursive: true }); + fs.writeFileSync(path.join(codexHome, 'config.toml'), content, 'utf8'); + } + + function runCodexInstall() { + const previousCodexHome = process.env.CODEX_HOME; + const previousCwd = process.cwd(); + process.env.CODEX_HOME = codexHome; + try { + process.chdir(path.join(__dirname, '..')); + return install(true, 'codex'); + } finally { + process.chdir(previousCwd); + if (previousCodexHome === undefined) { + delete process.env.CODEX_HOME; + } else { + process.env.CODEX_HOME = previousCodexHome; + } + } + } + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3285-')); + codexHome = path.join(tmpDir, 'codex-home'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('install does not throw when config.toml contains hooks.state trust entries', () => { + // This is the exact failure scenario reported in #3285. + const preInstall = [ + '[hooks.state]', + '', + "[hooks.state.'/home/user/.codex/hooks.json:pre_tool_use:0:0']", + 'enabled = true', + 'trusted_hash = "sha256:abc123def456"', + '', + ].join('\n'); + writeCodexConfig(preInstall); + + assert.doesNotThrow( + () => runCodexInstall(), + 'install must not throw when config.toml contains hooks.state trust entries' + ); + }); + + test('hooks.state entries are preserved in post-install config.toml', () => { + const preInstall = [ + '[hooks.state]', + '', + "[hooks.state.'/home/user/.codex/hooks.json:pre_tool_use:0:0']", + 'enabled = true', + 'trusted_hash = "sha256:abc123def456"', + '', + ].join('\n'); + writeCodexConfig(preInstall); + + runCodexInstall(); + + const after = fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8'); + // Verify structurally: the trust hash key must survive the install. + // Do NOT grep for the literal string — parse the TOML structure. + const { parseTomlToObject } = require('../bin/install.js'); + const parsed = parseTomlToObject(after); + assert.ok( + parsed.hooks && typeof parsed.hooks.state === 'object' && parsed.hooks.state !== null, + 'post-install config.toml must have hooks.state as an object' + ); + // Verify the actual trust entry survives — not just that hooks.state is an object. + const trustKey = "/home/user/.codex/hooks.json:pre_tool_use:0:0"; + assert.ok( + parsed.hooks.state[trustKey] != null, + `post-install must preserve the original trust entry for key: ${trustKey}` + ); + assert.strictEqual( + parsed.hooks.state[trustKey].enabled, + true, + 'preserved trust entry must have enabled = true' + ); + assert.strictEqual( + parsed.hooks.state[trustKey].trusted_hash, + 'sha256:abc123def456', + 'preserved trust entry must have the original trusted_hash' + ); + }); +});