Fix Codex hook installation to use hooks.json instead of inline TOML hooks#2640
Fix Codex hook installation to use hooks.json instead of inline TOML hooks#2640hanzckernel wants to merge 10 commits intogsd-build:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInstaller/uninstaller for Codex hooks now manage a GSD-owned SessionStart handler via Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer (bin/install.js)
participant FS as Filesystem
participant ConfigToml as config.toml
participant HooksJson as hooks.json (JSONC)
participant HooksDir as hooks/dist/ or hooks/
participant Manifest as gsd-file-manifest.json
Installer->>FS: resolve configDir
Installer->>ConfigToml: read config.toml
Installer->>HooksJson: read hooks.json (if present)
alt hooks.json parseable and safe
Installer->>HooksJson: remove prior managed entries
Installer->>HooksJson: merge managed SessionStart handler
HooksJson-->>FS: write updated hooks.json
else hooks.json malformed or unsafe
Installer->>ConfigToml: remove inline TOML only when clearly GSD-managed
Installer-->>FS: leave hooks.json bytes untouched and skip enabling codex_hooks
end
Installer->>HooksDir: copy hook asset (prefer hooks/dist/ then hooks/)
Installer->>Manifest: write/append hooks entry to gsd-file-manifest.json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
CHANGELOG.md (1)
24-24: Changelog entry is accurate but incomplete—missing key user-facing guarantees.The entry correctly describes the migration and the fix, but omits several important behaviors users need to know about:
- User hook preservation: The PR preserves user hooks that aren't the exact managed command and preserves malformed hooks.json content instead of rewriting it—these safety guarantees aren't mentioned.
- Selective file deletion: On uninstall, hooks.json is deleted only when it contains GSD-only content; otherwise user root keys are preserved—not mentioned.
- "Managed" definition: What constitutes a "managed hooks.json entry" (exact command match) vs user-owned hooks is unclear.
Users upgrading need reassurance their customizations won't be lost and their files won't be corrupted.
📝 Suggested expansion for completeness
-- **Codex install now writes the GSD SessionStart hook into `hooks.json` instead of inline `[[hooks]]` TOML** — fixes invalid `config.toml` shapes that made Codex reject model-setting updates with `invalid type: map, expected a sequence`; uninstall now removes only the managed `hooks.json` entry and source-checkout installs fall back to `hooks/` when `hooks/dist/` is absent. +- **Codex install now writes the GSD SessionStart hook into `hooks.json` instead of inline `[[hooks]]` TOML** — fixes invalid `config.toml` shapes that made Codex reject model-setting updates with `invalid type: map, expected a sequence`. Install merges the managed GSD SessionStart hook with existing hooks.json content without clobbering user-owned hooks, and preserves malformed existing hooks.json files instead of rewriting them. Uninstall removes only the exact managed GSD hook references from hooks.json (and legacy inline TOML blocks), preserves user root keys, and deletes the file only when it is GSD-only. Source-checkout installs fall back to `hooks/` when `hooks/dist/` is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 24, Update the changelog entry for the GSD SessionStart hook migration to explicitly state three user-facing guarantees: (1) user hooks are preserved unless they exactly match the managed GSD command (clarify the "managed" definition as an exact command string match to the GSD SessionStart hook), (2) malformed hooks.json content is preserved (we do not rewrite or normalize existing user files), and (3) uninstall deletes hooks.json only when it contains GSD-only managed entries and will preserve root keys or mixed user-owned entries; mention fallback behavior for source-checkout installs to hooks/ when hooks/dist/ is absent.bin/install.js (1)
6464-6497: Track the new Codex hook files in the manifest too.This branch now installs managed files under
hooks/, butwriteManifest()still skips Codex hooks. A local edit tohooks/gsd-check-update.jswill be wiped on reinstall without going through the existing patch-backup flow.♻️ Follow-up outside this hunk
- // Track hook files so saveLocalPatches() can detect user modifications - // Hooks are only installed for runtimes that use settings.json (not Codex/Copilot/Cline) - if (!isCodex && !isCopilot && !isCline) { + // Track hook files so saveLocalPatches() can detect user modifications. + // Codex now also installs managed hooks under hooks/. + if (!isCopilot && !isCline) { const hooksDir = path.join(configDir, 'hooks'); if (fs.existsSync(hooksDir)) { for (const file of fs.readdirSync(hooksDir)) { if (file.startsWith('gsd-') && (file.endsWith('.js') || file.endsWith('.sh'))) { manifest.files['hooks/' + file] = fileHash(path.join(hooksDir, file));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 6464 - 6497, writeManifest() currently skips the newly-installed Codex hook files so edits under hooks/ are not tracked; update the install flow to add those files to the manifest (or modify writeManifest to include the hooks/ directory) by recording each created hook path (codexHooksDest entries) with the same managed-file metadata used for other installed files, ensuring hooks/gsd-check-update.js and any .sh/.js files are added to the manifest so the patch-backup/restore logic applies on reinstall; reference the variables/functions codexHooksSrc, codexHooksDest, and writeManifest() and ensure the code path that writes files also invokes the manifest-add routine (or that writeManifest() enumerates targetDir/hooks) so hooks are tracked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/install.js`:
- Around line 2094-2099: The current loop filters lines by exact-line regex
/^\s*\[\[hooks\]\]\s*$/ which fails for valid TOML like '[[hooks]] # comment';
update the check inside the for-loop that examines line.text (the code around
variables lines and kept) to use a TOML header/table-array parser instead of the
strict regex: parse the line as a TOML header (or use an existing TOML header
utility) and treat any header that represents the table-array named "hooks"
(i.e., [[hooks]] even with trailing comments/whitespace) as the match to
skip/handle, so managed inline hook blocks are reliably detected during
install/uninstall cleanup.
In `@tests/codex-config.test.cjs`:
- Around line 1025-1053: The test currently writes malformed hooks with
writeCodexHooksJson() and only compares parsed JSON, which won't detect a
rewrite that preserves semantic content; instead, write a hand-authored
non-canonical JSON string for the hooks.json fixture (e.g., with extra spacing,
ordering, or comments-like whitespace) and write it directly to disk before
calling runCodexInstall(), then assert the raw file contents (via readFileSync
or an existing raw-reader helper) are byte-for-byte identical after install;
keep the existing parsed-JSON and config assertions (readCodexHooksJson,
readCodexConfig, countMatches) but add this raw-content equality check to ensure
install() does not reserialize/overwrite malformed hooks.json.
---
Nitpick comments:
In `@bin/install.js`:
- Around line 6464-6497: writeManifest() currently skips the newly-installed
Codex hook files so edits under hooks/ are not tracked; update the install flow
to add those files to the manifest (or modify writeManifest to include the
hooks/ directory) by recording each created hook path (codexHooksDest entries)
with the same managed-file metadata used for other installed files, ensuring
hooks/gsd-check-update.js and any .sh/.js files are added to the manifest so the
patch-backup/restore logic applies on reinstall; reference the
variables/functions codexHooksSrc, codexHooksDest, and writeManifest() and
ensure the code path that writes files also invokes the manifest-add routine (or
that writeManifest() enumerates targetDir/hooks) so hooks are tracked.
In `@CHANGELOG.md`:
- Line 24: Update the changelog entry for the GSD SessionStart hook migration to
explicitly state three user-facing guarantees: (1) user hooks are preserved
unless they exactly match the managed GSD command (clarify the "managed"
definition as an exact command string match to the GSD SessionStart hook), (2)
malformed hooks.json content is preserved (we do not rewrite or normalize
existing user files), and (3) uninstall deletes hooks.json only when it contains
GSD-only managed entries and will preserve root keys or mixed user-owned
entries; mention fallback behavior for source-checkout installs to hooks/ when
hooks/dist/ is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa9ae4b5-8276-47e3-b5f6-b1efa5ca6b0e
📒 Files selected for processing (3)
CHANGELOG.mdbin/install.jstests/codex-config.test.cjs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/install.js (1)
6480-6536:⚠️ Potential issue | 🟠 MajorRefresh the manifest after Codex hook installation.
The new hook-file tracking never sees the files copied here, because
writeManifest()still runs on Line 6419 before this Codex-only block.saveLocalPatches()will therefore miss user edits to Codex hook files on the next update.🔧 Proposed fix
} catch (e) { console.warn(` ${yellow}⚠${reset} Could not configure Codex hooks: ${e.message}`); } + + writeManifest(targetDir, runtime); return { settingsPath: null, settings: null, statuslineCommand: null, runtime, configDir: targetDir };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 6480 - 6536, The Codex hooks files are copied and hooks.json/config updated after writeManifest() ran earlier, so saveLocalPatches() misses those files; call writeManifest() again right after the Codex hooks block (after the console.log that prints "Configured Codex hooks (SessionStart via hooks.json)") so the manifest reflects the newly written hook files and saveLocalPatches() will pick them up; place the call adjacent to the existing code that uses buildHookCommand, mergeGsdIntoCodexHooksJson and stripManagedGsdCodexInlineHooks, and wrap it in a try/catch (or await if writeManifest is async) so any manifest refresh errors are logged but don’t break the install.
♻️ Duplicate comments (1)
bin/install.js (1)
2095-2096:⚠️ Potential issue | 🟡 MinorDecode TOML literal strings too.
JSON.parse()fixes basic strings, but TOML literal strings still leave doubled apostrophes intact. A command likecommand = 'node "/Users/O''Brien/.codex/hooks/gsd-check-update.js"'will missmanagedCommands, so the legacy[[hooks]]block survives migration/uninstall.🔧 Proposed fix
- const literalMatch = valueText.match(/^'(.*)'$/); - return literalMatch ? literalMatch[1] : null; + const literalMatch = valueText.match(/^'(.*)'$/); + return literalMatch ? literalMatch[1].replace(/''/g, "'") : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 2095 - 2096, The current literal string handling (using literalMatch on valueText) returns the inner content but doesn't un-escape TOML literal string doubled apostrophes, so values like node "/Users/O''Brien/..." remain incorrect; update the code path that handles literalMatch (the valueText.match(/^'(.*)'$/) branch) to return the captured content with TOML literal apostrophes decoded (replace doubled single quotes with a single quote) so managedCommands and hook paths are parsed correctly.
🧹 Nitpick comments (1)
tests/codex-config.test.cjs (1)
93-97: Minor readability improvement for theArray.isArraycheck.The expression
Array.isArray(group && group.hooks)works correctly but reads awkwardly. Whengroupis falsy, it evaluatesArray.isArray(false)which returnsfalse—correct but confusing.🔧 Suggested clarification
return Object.values(readCodexHooksJson(codexHome).hooks || {}) .flatMap(eventGroups => Array.isArray(eventGroups) ? eventGroups : []) - .flatMap(group => Array.isArray(group && group.hooks) ? group.hooks : []) + .flatMap(group => (group && Array.isArray(group.hooks)) ? group.hooks : []) .filter(hook => isManagedGsdUpdateHookCommand(hook && hook.command, codexHome)) .length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/codex-config.test.cjs` around lines 93 - 97, The Array.isArray(group && group.hooks) check is confusing when group is falsy; change it to explicitly check the property (e.g., use Array.isArray(group?.hooks) or group && Array.isArray(group.hooks)) so intent is clear; update the chain in the return expression that uses readCodexHooksJson(...).hooks, the flatMap that references group and group.hooks, and keep the rest of the pipeline (including isManagedGsdUpdateHookCommand) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/install.js`:
- Around line 5081-5098: The catch for cleaning hooks.json currently treats
failures as if the JSON were safely removed later; instead, when
stripGsdFromCodexHooksJson or fs.readFileSync throws, do NOT mark the repo as
having had GSD hooks removed nor allow the later removal of the managed hook
file (hooks/gsd-check-update.js). Concretely: in the catch block for the
hooksJsonPath handling, set a local flag like hooksJsonCleanFailed = true (or
rethrow) and do not increment removedCount; then update the later removal logic
that targets hooks/gsd-check-update.js to skip deletion if hooksJsonCleanFailed
is true (or abort when an error was thrown), so a malformed hooks.json isn't
left pointing to a deleted command.
---
Outside diff comments:
In `@bin/install.js`:
- Around line 6480-6536: The Codex hooks files are copied and hooks.json/config
updated after writeManifest() ran earlier, so saveLocalPatches() misses those
files; call writeManifest() again right after the Codex hooks block (after the
console.log that prints "Configured Codex hooks (SessionStart via hooks.json)")
so the manifest reflects the newly written hook files and saveLocalPatches()
will pick them up; place the call adjacent to the existing code that uses
buildHookCommand, mergeGsdIntoCodexHooksJson and
stripManagedGsdCodexInlineHooks, and wrap it in a try/catch (or await if
writeManifest is async) so any manifest refresh errors are logged but don’t
break the install.
---
Duplicate comments:
In `@bin/install.js`:
- Around line 2095-2096: The current literal string handling (using literalMatch
on valueText) returns the inner content but doesn't un-escape TOML literal
string doubled apostrophes, so values like node "/Users/O''Brien/..." remain
incorrect; update the code path that handles literalMatch (the
valueText.match(/^'(.*)'$/) branch) to return the captured content with TOML
literal apostrophes decoded (replace doubled single quotes with a single quote)
so managedCommands and hook paths are parsed correctly.
---
Nitpick comments:
In `@tests/codex-config.test.cjs`:
- Around line 93-97: The Array.isArray(group && group.hooks) check is confusing
when group is falsy; change it to explicitly check the property (e.g., use
Array.isArray(group?.hooks) or group && Array.isArray(group.hooks)) so intent is
clear; update the chain in the return expression that uses
readCodexHooksJson(...).hooks, the flatMap that references group and
group.hooks, and keep the rest of the pipeline (including
isManagedGsdUpdateHookCommand) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b4101ad-bd3b-47e3-90e5-e29d0db89f1a
📒 Files selected for processing (2)
bin/install.jstests/codex-config.test.cjs
Refresh the Codex hook manifest after Codex-specific hook installation, decode doubled apostrophes when matching legacy TOML literal commands, and preserve the update hook file when hooks.json cannot be cleaned during uninstall.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/install.js (1)
5308-5318:⚠️ Potential issue | 🟡 Minor
gsd-check-update-worker.jsis still left behind on uninstall.Codex install now copies and manifests the full hook set, but this deletion list still omits
hooks/gsd-check-update-worker.js, so uninstall leaves a managed GSD artifact behind.🔧 Proposed fix
- const gsdHooks = ['gsd-statusline.js', 'gsd-check-update.js', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', 'gsd-read-guard.js', 'gsd-read-injection-scanner.js', 'gsd-workflow-guard.js', 'gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh']; + const gsdHooks = ['gsd-statusline.js', 'gsd-check-update.js', 'gsd-check-update-worker.js', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', 'gsd-read-guard.js', 'gsd-read-injection-scanner.js', 'gsd-workflow-guard.js', 'gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh']; @@ - if (isCodex && codexHooksJsonCleanFailed && hook === 'gsd-check-update.js') { + if (isCodex && codexHooksJsonCleanFailed && (hook === 'gsd-check-update.js' || hook === 'gsd-check-update-worker.js')) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 5308 - 5318, The uninstall hook list (gsdHooks array in bin/install.js) omits the managed worker file gsd-check-update-worker.js so it isn't removed; add 'gsd-check-update-worker.js' to the gsdHooks array (or otherwise ensure the same deletion logic that uses hooksDir, isCodex, codexHooksJsonCleanFailed, and the hook loop applies to that filename) so the file is checked for existence and unlinked during the uninstall pass.
🧹 Nitpick comments (1)
tests/codex-config.test.cjs (1)
75-97: Align managed-hook command matching with installer variants.At Line [75], the helper only tracks quoted absolute command forms. The installer’s managed-command matcher supports quoted/unquoted and
$HOME-portable forms too, socountGsdUpdateHooksInHooksJson()(Line [87]) can miss valid managed entries.♻️ Suggested parity update
function getManagedGsdUpdateHookCommands(codexHome) { - const hooksDir = path.join(codexHome, 'hooks').replace(/\\/g, '/'); - return new Set([ - `node "${hooksDir}/gsd-check-update.js"`, - `node "${hooksDir}/gsd-update-check.js"`, - ]); + const normalized = codexHome.replace(/\\/g, '/'); + const home = os.homedir().replace(/\\/g, '/'); + const portableBase = normalized.startsWith(home) + ? '$HOME' + normalized.slice(home.length) + : normalized; + + const commands = new Set(); + for (const hookName of ['gsd-check-update.js', 'gsd-update-check.js']) { + for (const baseDir of new Set([normalized, portableBase])) { + const hookPath = `${baseDir}/hooks/${hookName}`; + commands.add(`node ${hookPath}`); + commands.add(`node "${hookPath}"`); + } + } + return commands; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/codex-config.test.cjs` around lines 75 - 97, The current helper only recognizes quoted absolute command strings in getManagedGsdUpdateHookCommands, causing isManagedGsdUpdateHookCommand/countGsdUpdateHooksInHooksJson to miss installer-supported variants; update getManagedGsdUpdateHookCommands and/or isManagedGsdUpdateHookCommand to accept both quoted and unquoted forms and $HOME-portable forms by generating or matching variants for node "<hooksDir>/gsd-*.js", node <hooksDir>/gsd-*.js, node "$HOME/..../gsd-*.js" and node $HOME/..../gsd-*.js (or use a regex that normalizes optional quotes and replaces codexHome with \$HOME) so countGsdUpdateHooksInHooksJson correctly detects managed hooks across installer variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/install.js`:
- Around line 2066-2071: The matcher for legacy hook commands currently only
adds bare (`node ${hookPath}`) and double-quoted (`node "${hookPath}"`) forms;
add the single-quoted form as well so decoded TOML literal strings like `node
'$HOME/.../gsd-check-update.js'` are recognized. In the loop that iterates
hookName and baseDir (referencing hookName, baseDir, normalized, portableBase)
add a third commands.add entry that constructs the single-quoted command (e.g.,
commands.add(`node '${hookPath}'`)) alongside the existing two variants so the
single-quoted legacy form is cleaned up too.
- Around line 2306-2310: The code treats a zero-byte existingContent as falsy
and behaves as if the file is missing; update mergeGsdIntoCodexHooksJson to
distinguish "no file" (null/undefined) from an empty string by using explicit
null/undefined checks (e.g., existingContent != null) when computing eol
(detectLineEnding) and parsed (parseCodexHooksJson) so an empty hooks.json is
preserved rather than being replaced; reference mergeGsdIntoCodexHooksJson, eol,
parsed, detectLineEnding, and parseCodexHooksJson when making the change.
---
Outside diff comments:
In `@bin/install.js`:
- Around line 5308-5318: The uninstall hook list (gsdHooks array in
bin/install.js) omits the managed worker file gsd-check-update-worker.js so it
isn't removed; add 'gsd-check-update-worker.js' to the gsdHooks array (or
otherwise ensure the same deletion logic that uses hooksDir, isCodex,
codexHooksJsonCleanFailed, and the hook loop applies to that filename) so the
file is checked for existence and unlinked during the uninstall pass.
---
Nitpick comments:
In `@tests/codex-config.test.cjs`:
- Around line 75-97: The current helper only recognizes quoted absolute command
strings in getManagedGsdUpdateHookCommands, causing
isManagedGsdUpdateHookCommand/countGsdUpdateHooksInHooksJson to miss
installer-supported variants; update getManagedGsdUpdateHookCommands and/or
isManagedGsdUpdateHookCommand to accept both quoted and unquoted forms and
$HOME-portable forms by generating or matching variants for node
"<hooksDir>/gsd-*.js", node <hooksDir>/gsd-*.js, node "$HOME/..../gsd-*.js" and
node $HOME/..../gsd-*.js (or use a regex that normalizes optional quotes and
replaces codexHome with \$HOME) so countGsdUpdateHooksInHooksJson correctly
detects managed hooks across installer variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc5ded1a-9cbc-461c-adcf-21bed2689081
📒 Files selected for processing (3)
CHANGELOG.mdbin/install.jstests/codex-config.test.cjs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/codex-config.test.cjs (1)
75-97: Align test managed-command matching with production variantsOn Line 75,
getManagedGsdUpdateHookCommandsonly matches double-quoted absolute commands. Production cleanup logic matches additional forms (unquoted, single-quoted, and$HOME-portable), so this helper can undercount managed hooks and miss regression cases.Proposed fix
function getManagedGsdUpdateHookCommands(codexHome) { - const hooksDir = path.join(codexHome, 'hooks').replace(/\\/g, '/'); - return new Set([ - `node "${hooksDir}/gsd-check-update.js"`, - `node "${hooksDir}/gsd-update-check.js"`, - ]); + const home = os.homedir().replace(/\\/g, '/'); + const normalized = codexHome.replace(/\\/g, '/'); + const portableBase = normalized.startsWith(home) + ? '$HOME' + normalized.slice(home.length) + : normalized; + + const commands = new Set(); + for (const hookName of ['gsd-check-update.js', 'gsd-update-check.js']) { + for (const baseDir of new Set([normalized, portableBase])) { + const hookPath = `${baseDir}/hooks/${hookName}`; + commands.add(`node ${hookPath}`); + commands.add(`node "${hookPath}"`); + commands.add(`node '${hookPath}'`); + } + } + return commands; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/codex-config.test.cjs` around lines 75 - 97, getManagedGsdUpdateHookCommands currently only returns double-quoted absolute command strings so tests miss other forms the production cleanup handles; update getManagedGsdUpdateHookCommands to generate and return all equivalent variants for each script (double-quoted, single-quoted, unquoted) and versions that use $HOME instead of the absolute codexHome path, then keep using isManagedGsdUpdateHookCommand and countGsdUpdateHooksInHooksJson as-is so the filter will correctly recognize managed hooks in their alternative forms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/install.js`:
- Around line 6527-6543: The current success log runs even when
ensureCodexHooksFeature() returned an inline-table ownership (e.g., features = {
codex_hooks = true }) which prevents migration because
hasEnabledCodexHooksFeature() stays false; detect that case by checking
codexHooksFeature.ownership (or the equivalent ownership flag returned by
ensureCodexHooksFeature) and treat it as a non-migrated inline-table case:
either perform the conversion path (so
stripManagedGsdCodexInlineHooks/mergeGsdIntoCodexHooksJson run) or, if
conversion isn’t done automatically, emit a warning and skip printing the
success message. Concretely, update the conditional after computing
codexHooksFeature and nextConfigContent to branch when
codexHooksFeature.ownership === 'inline-table' (or the actual ownership token)
and handle that case (run migration or log a warning and avoid the “Configured
Codex hooks” console.log) so we don’t claim success when migration cannot run.
---
Nitpick comments:
In `@tests/codex-config.test.cjs`:
- Around line 75-97: getManagedGsdUpdateHookCommands currently only returns
double-quoted absolute command strings so tests miss other forms the production
cleanup handles; update getManagedGsdUpdateHookCommands to generate and return
all equivalent variants for each script (double-quoted, single-quoted, unquoted)
and versions that use $HOME instead of the absolute codexHome path, then keep
using isManagedGsdUpdateHookCommand and countGsdUpdateHooksInHooksJson as-is so
the filter will correctly recognize managed hooks in their alternative forms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e120fdfc-2b3a-40c9-8fd6-e2abfe52e74a
📒 Files selected for processing (2)
bin/install.jstests/codex-config.test.cjs
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Adversarial review — request changes
Verdict
Request changes. The migration is well-tested and the edge-case handling (malformed hooks.json, zero-byte file, TOML literal apostrophes, managed-command cleanup across event names) is careful work. Two concrete problems before merge.
RCA — one gap
Missing version guard. Issue #2637 explicitly asked for "detect affected Codex versions and skip enabling hooks" or equivalent mitigation. This PR unconditionally switches every Codex install to hooks.json (bin/install.js:6606-6636). Older Codex releases that only understood the inline [[hooks]] TOML are now silently unhooked — the SessionStart update check stops firing because the hooks.json format didn't exist yet. No runtime/version detection, no fallback. Gall's law: the new format works for 0.124.0, but you've replaced a working-for-old-Codex system with one that assumes everyone is on the hooks.json-aware build. Goodhart: the test "does not write legacy inline TOML hooks" (tests/codex-config.test.cjs:769-780) optimizes for a metric (no [[hooks]]) that is exactly what older Codex needs. Either probe codex --version or detect hooks.json support capability — the README-level promise of "update checks via SessionStart" is now false on <0.124.0 without telling the user.
Rubber-duck — followups that look like smells
getManagedCodexSessionStartCommands(bin/install.js:2419-2421) is a one-line alias ofgetManagedCodexLegacyInlineCommands. Kernighan: the alias obscures that session-start-json and legacy-inline-toml commands are the same string set — delete the alias or give it a reason to exist. Peter's principle applied to function names: they rise to a level of abstraction past their usefulness.bin/install.js:5975-5977quietly widenswriteManifesthook tracking from!isCodex && !isCopilot && !isClineto!isCopilot && !isCline. Correct direction, but out of scope for a PR titled "hooks.json migration" — mention it in the PR body or split. Knuth: premature generalization of the gate is how test coverage for non-Codex runtimes silently drifts.
Blame
Inline [[hooks]] TOML was introduced in 70f1f8b "feat: add Codex hooks support for SessionStart (#1020)" — Tom Boucher, 2026-03-17, +30 lines to bin/install.js. That commit hardcoded the now-deprecated format.
Required changes
- Add Codex version detection (or capability probe for
hooks.jsonsupport) and fall back to legacy[[hooks]]write on older Codex. Skip hook install entirely on ambiguous detection rather than silently breaking update checks. - Inline or rename
getManagedCodexSessionStartCommandsso the call site atbin/install.js:2438/2506reads honestly. - Note the
writeManifestgate change in the PR body (or split).
CI is green. Diff is +1106/−200 across 4 files, tests are the bulk of it, and the test matrix is good. Fix the version guard and this ships.
trek-e
added
bug
Detect the installed Codex CLI before configuring update hooks. Keep legacy inline SessionStart hooks for Codex versions before 0.124.0, use hooks.json for 0.124.0 and newer, and skip hook configuration when support cannot be determined safely. Add regression coverage for version selection, legacy fallback, and skip mode.
|
Addressed.
Verified: |
…n-migration # Conflicts: # CHANGELOG.md # bin/install.js
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Code Review: Fix Codex hook installation to use hooks.json instead of inline TOML hooks
1. Peter Principle concern — 400+ lines of Codex internals from a first-time contributor
This PR adds: version detection via spawnSync, custom semver comparison, a partial TOML parser, hooks.json read/write/merge logic with path normalization, and a 6-variant command enumeration system for uninstall. That is the implementation surface of an experienced systems programmer who has reverse-engineered Codex's config format in depth. What is the source of the contributor's knowledge of Codex's hooks.json schema, version detection API, and migration behavior? Has any of this been validated against Codex's actual behavior, or is it reverse-engineered from observation?
2. Kernighan's Law: getManagedCodexUpdateHookCommands generates 6 variants because install wrote inconsistent paths
The uninstall function generates 6 command variants (normalized × portable base × 2 hook names × multiple quoting styles) to find what the install function wrote. This is a symptom, not a fix. The root cause is that the install path was written in an inconsistent format. The correct fix is: install writes ONE canonical path, uninstall removes that exact path. Enumerating all possible variants is self-documenting evidence that the install side was not fixed. When does the install path format diverge? Fix that, and the uninstall complexity collapses.
3. parseSimpleTomlStringAssignment is a partial TOML parser that fails silently
The function handles double-quoted and single-quoted strings. TOML also has: multi-line basic strings ("""), multi-line literal strings ('''), and escaped characters within strings (\", \\, \n, etc.). Adjacent keys in a real Codex TOML config may use any of these. When this parser encounters syntax it doesn't recognize, it returns null — silently. A caller that receives null from this function cannot distinguish "key not found" from "key found but unparseable." Silent null returns on parse failure will cause undetected migration failures.
4. detectCodexHookInstallMode conflates "Codex not installed" with "version check failed"
The function returns mode: 'skip' in both cases. On a machine where Codnet is installed but codex --version fails due to a PATH issue or permissions error, the GSD Codex hook is silently skipped — not installed, no error, no warning. The distinction between "Codex is not present" and "Codex is present but misconfigured" has different user-facing implications. One should skip silently; the other should warn.
5. GSD_CODEX_HOOKS_MODE env var is now a permanent production API surface
Adding an env var override to force production code into a test mode is a code smell. This env var will be discovered, documented (or not), and used by users who want to force a specific install behavior. You've created an undocumented knob with production side effects. This should be test infrastructure only — use dependency injection (pass installMode as a parameter) instead of an env var that bleeds into production.
6. The complexity-to-benefit ratio has not been justified
The old system had one failure mode: Codex rejected inline TOML hooks. The new system has: version detection failure, spawnSync unavailability, hooks.json read failure, hooks.json parse failure, hooks.json write failure, path normalization edge cases, partial TOML parse failure, and 6-variant uninstall mismatch. Gall's Law: a complex system that works must evolve from a simple system that worked. This PR replaced a simple system that had one known failure mode with a complex system that has eight. What evidence exists that the complex system handles all eight failure modes correctly?
VERDICT: CHANGES REQUIRED — fix the install path inconsistency at the root (eliminating the need for 6-variant uninstall), remove the production env var, and validate against actual Codex behavior.
|
Closing this PR. A review identified more than 3 distinct problems with the implementation that collectively indicate it is not ready for merge in its current form. The specific issues are documented in the review comment above — please address all of them in a revised PR rather than pushing fixes to this branch. If you believe any finding is incorrect, open a new PR with a clear rebuttal for each point. |
2 participants
Fix PR
Linked Issue
Fixes #2637
What was broken
GSD installed a Codex
SessionStartupdate hook as inline[[hooks]]TOML inconfig.toml. Newer Codex versions expect hooks inhooks.json, so model-setting updates could fail withinvalid type: map, expected a sequence. The install/uninstall path also had edge cases where managed hook references could survive in malformed or legacy configs.What this fix does
Moves the managed Codex update hook to
hooks.json, preserves user-owned hook content, avoids rewriting malformed existinghooks.jsonfiles, tracks Codex hook files in the manifest, and removes only exact managed GSD hook references during uninstall across bothhooks.jsonand legacy inline TOML hook blocks.Root cause
The installer treated Codex hook configuration like inline TOML hook arrays instead of Codex’s
hooks.jsonformat, and the cleanup logic matched only narrower legacy shapes than the real world configs users could carry forward.Testing
How I verified the fix
hooks.jsonmigration and merge behaviorhooks.jsonpreservation without rewriteSessionStart/inline TOML casesfeatures = { codex_hooks = true }configs migrate tohooks.jsonwithout invalid dotted-key rewritesnpm run build:hooks && node --test tests/codex-config.test.cjsgit diff --checknpm testRegression test added?
Platforms tested
Runtimes tested
Checklist
Fixes #NNNconfirmed-buglabelnpm test)Breaking changes
None
Summary by CodeRabbit