fix(#2637): write Codex SessionStart hook to hooks.json, not config.toml#2763
fix(#2637): write Codex SessionStart hook to hooks.json, not config.toml#2763MiaoDX wants to merge 1 commit 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:
📝 WalkthroughWalkthroughMoves GSD-managed Codex hook registration from inline Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,201,0.5)
actor User
end
rect rgba(187,222,251,0.5)
participant Installer
end
rect rgba(255,224,178,0.5)
participant FS as ".codex/hooks.json / config.toml"
end
rect rgba(255,204,204,0.5)
participant Codex
end
User->>Installer: run install/uninstall (--codex)
Installer->>FS: read config.toml
Installer->>FS: read hooks.json (if exists)
alt malformed or unsupported hooks.json
FS-->>Installer: parse error / unexpected root
Installer-->>User: emit warning, skip JSON modification
else valid or absent hooks.json
Installer->>FS: strip legacy [[hooks]] / # GSD Hooks from config.toml
Installer->>FS: upsert/remove managed SessionStart entry in hooks.json
FS-->>Installer: write updated hooks.json (or delete if empty)
end
Installer->>Codex: ensure features.codex_hooks flag in config.toml
Installer-->>User: finish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
tests/codex-config.test.cjs (2)
79-93: Optional: tighten the substring match used to identify the managed GSD hook.
h.command.includes('gsd-check-update')will also count any user-authored hook command that happens to contain that substring (unlikely, but possible). Since the canonical install writesnode <absolute>/hooks/gsd-check-update.js, anchoring on the script filename or matching the trailinggsd-check-update.jsis more defensive and won't conflate with a hypothetical user hook that mentions the string in its arguments.♻️ Suggested tightening
- if (h && typeof h.command === 'string' && h.command.includes('gsd-check-update')) count++; + if (h && typeof h.command === 'string' && /\bgsd-check-update\.js\b/.test(h.command)) count++;🤖 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 79 - 93, The hook detection in countCodexGsdHooks is too loose (h.command.includes('gsd-check-update')); update the check in function countCodexGsdHooks so it matches the managed script filename rather than any substring—e.g., replace the includes check with a stricter test on h.command such as matching the filename (e.g., endsWith('gsd-check-update.js') or using a regex like /\bgsd-check-update(?:\.js)?\b/) to reliably identify the installed /hooks/gsd-check-update script.
1860-1886: Uninstall symmetry tests look solid; consider one extra edge case.The two tests cover the documented uninstall behavior well (preserve user entries, delete the file when GSD was the sole entry). One additional case worth a follow-up test: a user-authored entry exists in a different event (e.g. only
UserPromptSubmit) while GSD owns the onlySessionStartentry — confirming uninstall removes the entire emptySessionStartarray rather than leavingSessionStart: []behind, and keepsUserPromptSubmitintact. Not blocking, just a corner case that closes the loop on the merge/unmerge symmetry.🤖 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 1860 - 1886, Add a new test that covers the edge case where a user-authored hook exists under a different event (e.g., UserPromptSubmit) while GSD owns the only entry for SessionStart; use runCodexInstall(codexHome) to install, then runCodexUninstall(codexHome) to uninstall, then call readCodexHooksJson(codexHome) and countCodexGsdHooks(codexHome) to assert that the GSD hooks are removed, the SessionStart key is not left as an empty array/object (i.e., no empty SessionStart entry remains), and the user hook under UserPromptSubmit is still present; mirror the style and assertions used in the existing tests for consistency.
🤖 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 2869-2872: The current deletion checks only data.hooks and removes
hooksJsonPath even if other top-level keys exist; change the condition to delete
the file only when the entire data object is empty (e.g., check
Object.keys(data).length === 0) and otherwise write the updated data back to
hooksJsonPath. Locate the block that references data.hooks and hooksJsonPath and
replace the conditional so the file is unlinked only if the whole data object
has no keys; otherwise call fs.writeFileSync(...) as before.
- Around line 2804-2808: The code allows JSON arrays to be treated as objects
when loading hooks.json (parsed passes typeof === 'object'), which later causes
data.hooks mutations to be dropped; update the parse/assignment logic around
hooksJsonPath so that after JSON.parse you verify parsed is a non-null plain
object (e.g., typeof parsed === 'object' && parsed !== null &&
!Array.isArray(parsed')) before setting data = parsed, thereby rejecting array
roots and preventing data.hooks from being lost; adjust the conditional that
assigns to data (and any code that assumes data is an object) to rely on this
stricter check.
- Around line 6516-6522: The success log currently always prints after
attempting to add the SessionStart hook; change this to check the boolean result
returned by upsertManagedCodexHookInHooksJson(hooksJsonPath, 'SessionStart',
command) and only print the success message when that call returns true; when it
returns false (malformed/unsupported hooks.json) either skip the success log or
emit a warning/notice about skipping the hook. Locate the block gated by
hasEnabledCodexHooksFeature(configContent) and use the return value of
upsertManagedCodexHookInHooksJson to decide whether to call console.log(`
${green}✓${reset} Configured Codex hooks (SessionStart)`) or to log an
appropriate skipped/failed message.
---
Nitpick comments:
In `@tests/codex-config.test.cjs`:
- Around line 79-93: The hook detection in countCodexGsdHooks is too loose
(h.command.includes('gsd-check-update')); update the check in function
countCodexGsdHooks so it matches the managed script filename rather than any
substring—e.g., replace the includes check with a stricter test on h.command
such as matching the filename (e.g., endsWith('gsd-check-update.js') or using a
regex like /\bgsd-check-update(?:\.js)?\b/) to reliably identify the installed
/hooks/gsd-check-update script.
- Around line 1860-1886: Add a new test that covers the edge case where a
user-authored hook exists under a different event (e.g., UserPromptSubmit) while
GSD owns the only entry for SessionStart; use runCodexInstall(codexHome) to
install, then runCodexUninstall(codexHome) to uninstall, then call
readCodexHooksJson(codexHome) and countCodexGsdHooks(codexHome) to assert that
the GSD hooks are removed, the SessionStart key is not left as an empty
array/object (i.e., no empty SessionStart entry remains), and the user hook
under UserPromptSubmit is still present; mirror the style and assertions used in
the existing tests for consistency.
🪄 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: e3011e39-df4d-4f5f-85dd-9a075fa48d61
📒 Files selected for processing (4)
CHANGELOG.mdbin/install.jstests/bug-2698-crlf-install.test.cjstests/codex-config.test.cjs
- Reject non-plain-object roots (arrays, scalars) in hooks.json before
merging — JSON.parse('[]') passes typeof === 'object', causing data
to be set to an array and the .hooks mutation to be silently dropped
by JSON.stringify. Now skip cleanly with a `false` return.
- Preserve sibling top-level keys on uninstall — only unlink hooks.json
when the entire root object is empty. Prior code deleted the file as
soon as `data.hooks` was empty, nuking user metadata (version markers,
custom keys) at the same level.
- Surface skipped hooks.json updates — when upsert returns false, log a
warning instead of the green ✓ "Configured Codex hooks (SessionStart)"
success line.
Tests: two new e2e cases covering array-rooted hooks.json (preserved
verbatim, no GSD hook written) and uninstall with sibling top-level
keys (sibling keys preserved, empty `hooks` removed). All 5652 tests
pass.
Refs: PR gsd-build#2763 review by coderabbitai.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/codex-config.test.cjs (3)
79-93: Optional: import the shared marker constant instead of duplicating the literal.
countCodexGsdHookshardcodes'gsd-check-update'whilebin/install.jsexports/uses aGSD_CODEX_HOOK_MARKERconstant (see snippet atbin/install.js:2787-2790) for the exact same purpose. If the marker is ever changed (or the script renamed), this test helper will silently start counting zero. Consider exportingGSD_CODEX_HOOK_MARKERfrombin/install.jsand importing it here so the test stays in sync with production.♻️ Proposed fix
const { getCodexSkillAdapterHeader, ... GSD_CODEX_MARKER, + GSD_CODEX_HOOK_MARKER, CODEX_AGENT_SANDBOX, } = require('../bin/install.js');for (const h of entry.hooks) { - if (h && typeof h.command === 'string' && h.command.includes('gsd-check-update')) count++; + if (h && typeof h.command === 'string' && h.command.includes(GSD_CODEX_HOOK_MARKER)) count++; }🤖 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 79 - 93, The test helper countCodexGsdHooks currently hardcodes the marker string 'gsd-check-update'; update it to import and use the canonical GSD_CODEX_HOOK_MARKER exported from bin/install.js so the test stays in sync with production. Modify the file to add an import of GSD_CODEX_HOOK_MARKER from bin/install.js and then replace the literal check in countCodexGsdHooks (the condition testing h.command.includes('gsd-check-update')) to use GSD_CODEX_HOOK_MARKER instead.
1912-1918: Consider also asserting thehooks/directory and the hook script itself are cleaned up (or intentionally retained).This test confirms
hooks.jsonis removed when GSD was its only content, but it doesn't check what happens tohooks/gsd-check-update.js(installed at line 1177 as a separate concern). If uninstall is meant to be symmetric with install, a small assertion either way (removed, or explicitly retained) would lock down the contract and prevent future drift.🤖 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 1912 - 1918, The test 'hooks.json: uninstall deletes hooks.json when GSD was its only entry' currently only asserts hooks.json is removed; add assertions after runCodexInstall/runCodexUninstall to check the hooks directory and the specific hook script to lock the contract: verify that path.join(codexHome, 'hooks') exists (or does not) and that path.join(codexHome, 'hooks', 'gsd-check-update.js') is either removed or explicitly retained according to the intended behavior; use the existing helpers runCodexInstall and runCodexUninstall and the same fs.existsSync-based pattern to assert the expected state for both the directory ('hooks') and the file ('gsd-check-update.js').
73-77: Optional: makereadCodexHooksJsonresilient to malformed content.
JSON.parsewill throw ifhooks.jsonis malformed. All callers in this file currently invoke it only after a successful install (which writes valid JSON) or after seeding a syntactically valid file, so this is not exercised today — but the malformed-file scenario tested at lines 1880–1890 deliberately bypasses this helper. A small try/catch returningnullwould let the helper be reused everywhere uniformly.♻️ Proposed fix
function readCodexHooksJson(codexHome) { const p = path.join(codexHome, 'hooks.json'); if (!fs.existsSync(p)) return null; - return JSON.parse(fs.readFileSync(p, 'utf8')); + try { + return JSON.parse(fs.readFileSync(p, 'utf8')); + } catch { + return null; + } }🤖 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 73 - 77, The helper readCodexHooksJson currently calls JSON.parse directly which will throw on malformed hooks.json; wrap the JSON.parse/fs.readFileSync call in a try/catch inside readCodexHooksJson (keeping the existing exists check) and return null if parsing or read fails so callers get null instead of an exception; update only the readCodexHooksJson function to catch errors and return null while preserving its existing behavior when the file is absent.
🤖 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 6530-6532: The constructed command string currently omits quotes
around updateCheckScript causing paths with spaces to break; update the code
that builds command (where updateCheckScript and command are defined) to quote
the script path before passing to upsertManagedCodexHookInHooksJson (e.g., wrap
updateCheckScript in double quotes or use a safe serializer like
JSON.stringify(updateCheckScript)) so the stored SessionStart hook is "node
\"<path>\"" and works for dirs with spaces.
- Around line 2816-2819: The current logic forcibly normalizes malformed hooks
payloads by replacing data.hooks with {} and data.hooks[eventName] with [] which
can erase user data; change it to detect unsupported shapes and skip
handling/writing the hooks.json instead of mutating it: if data.hooks is
missing, null, not an object, or is an array, or if data.hooks[eventName] exists
but is not an array, bail out/return early and leave the file untouched; only
create or mutate data.hooks and data.hooks[eventName] when data.hooks is an
ordinary object and data.hooks[eventName] is absent or already an array (use the
symbols data.hooks and eventName to locate the checks).
---
Nitpick comments:
In `@tests/codex-config.test.cjs`:
- Around line 79-93: The test helper countCodexGsdHooks currently hardcodes the
marker string 'gsd-check-update'; update it to import and use the canonical
GSD_CODEX_HOOK_MARKER exported from bin/install.js so the test stays in sync
with production. Modify the file to add an import of GSD_CODEX_HOOK_MARKER from
bin/install.js and then replace the literal check in countCodexGsdHooks (the
condition testing h.command.includes('gsd-check-update')) to use
GSD_CODEX_HOOK_MARKER instead.
- Around line 1912-1918: The test 'hooks.json: uninstall deletes hooks.json when
GSD was its only entry' currently only asserts hooks.json is removed; add
assertions after runCodexInstall/runCodexUninstall to check the hooks directory
and the specific hook script to lock the contract: verify that
path.join(codexHome, 'hooks') exists (or does not) and that path.join(codexHome,
'hooks', 'gsd-check-update.js') is either removed or explicitly retained
according to the intended behavior; use the existing helpers runCodexInstall and
runCodexUninstall and the same fs.existsSync-based pattern to assert the
expected state for both the directory ('hooks') and the file
('gsd-check-update.js').
- Around line 73-77: The helper readCodexHooksJson currently calls JSON.parse
directly which will throw on malformed hooks.json; wrap the
JSON.parse/fs.readFileSync call in a try/catch inside readCodexHooksJson
(keeping the existing exists check) and return null if parsing or read fails so
callers get null instead of an exception; update only the readCodexHooksJson
function to catch errors and return null while preserving its existing behavior
when the file 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: 007ae7c9-21a7-46ae-82d1-9ac8e92240e5
📒 Files selected for processing (2)
bin/install.jstests/codex-config.test.cjs
- Reject non-plain-object roots (arrays, scalars) in hooks.json before
merging — JSON.parse('[]') passes typeof === 'object', causing data
to be set to an array and the .hooks mutation to be silently dropped
by JSON.stringify. Now skip cleanly with a `false` return.
- Preserve sibling top-level keys on uninstall — only unlink hooks.json
when the entire root object is empty. Prior code deleted the file as
soon as `data.hooks` was empty, nuking user metadata (version markers,
custom keys) at the same level.
- Surface skipped hooks.json updates — when upsert returns false, log a
warning instead of the green ✓ "Configured Codex hooks (SessionStart)"
success line.
Tests: two new e2e cases covering array-rooted hooks.json (preserved
verbatim, no GSD hook written) and uninstall with sibling top-level
keys (sibling keys preserved, empty `hooks` removed). All 5652 tests
pass.
Refs: PR gsd-build#2763 review by coderabbitai.
MiaoDX
force-pushed
the
fix/2637-codex-hooks-json
branch
from
efc05de to
a1e2d48
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
bin/install.js (2)
2824-2827:⚠️ Potential issue | 🟠 MajorDon't rewrite unsupported
hooks.jsonpayloads.Normalizing
data.hooks/data.hooks[eventName]here turns malformed-but-user-owned files into partially erased documents. This helper should skip those shapes and leave the file untouched instead of coercing them into{}/[].Suggested fix
- if (!data.hooks || typeof data.hooks !== 'object' || Array.isArray(data.hooks)) { - data.hooks = {}; - } - if (!Array.isArray(data.hooks[eventName])) data.hooks[eventName] = []; + if (data.hooks === undefined) { + data.hooks = {}; + } else if (!data.hooks || typeof data.hooks !== 'object' || Array.isArray(data.hooks)) { + return false; + } + if (data.hooks[eventName] === undefined) { + data.hooks[eventName] = []; + } else if (!Array.isArray(data.hooks[eventName])) { + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 2824 - 2827, The current normalization unconditionally replaces malformed user-owned hooks payloads; instead, change the logic in the helper around `data.hooks` and `data.hooks[eventName]` so we only create or mutate when the shape is absent, not when it's present but invalid. Specifically: if `data.hooks` is undefined, initialize it to {}; if `data.hooks` already exists but is not an object or is an array, do not modify it (leave the file as-is). Likewise, only set `data.hooks[eventName] = []` when `data.hooks[eventName]` is strictly undefined; if it exists but is not an array, do not coerce it. Apply these checks where the current `if (!data.hooks || typeof data.hooks !== 'object' || Array.isArray(data.hooks))` and `if (!Array.isArray(data.hooks[eventName]))` logic appears.
6572-6574:⚠️ Potential issue | 🟠 MajorReuse
buildHookCommand()for the SessionStart command.The raw
node ${updateCheckScript}string breaks whenCODEX_HOME/--config-dircontains spaces, and it also bypasses the existing portable-path handling.Suggested fix
- const updateCheckScript = path.resolve(targetDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); - const command = `node ${updateCheckScript}`; + const command = buildHookCommand(targetDir, 'gsd-check-update.js', { + portableHooks: hasPortableHooks, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 6572 - 6574, Replace the raw command string that builds `node ${updateCheckScript}` with a call to the existing buildHookCommand helper so path quoting/portable-path handling is reused; specifically, when constructing the SessionStart command before calling upsertManagedCodexHookInHooksJson, call buildHookCommand with the resolved updateCheckScript (the same value currently in updateCheckScript) and pass that result as the command to upsertManagedCodexHookInHooksJson instead of the inline `node ...` string.
🧹 Nitpick comments (5)
tests/codex-config.test.cjs (3)
1912-1918: Optional: tighten the “GSD-only ⇒ unlink” assertion.The test confirms
hooks.jsonis removed after uninstall, but doesn’t differentiate “file removed because GSD was the only entry” from “file removed for some other reason.” Consider a complementary negative case (e.g., a fresh install plus a sibling top-level key likeversion) to assert the file is kept — the symmetric counterpart to Lines 1861–1878. Not blocking; the existing coverage already exercises both branches across the suite.🤖 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 1912 - 1918, Add a complementary negative test that ensures hooks.json is preserved when it contains other top-level entries: after runCodexInstall(codexHome) write or merge an extra top-level key (e.g., "version" or "sibling") into the hooks.json file, then call runCodexUninstall(codexHome) and assert that fs.existsSync(path.join(codexHome, 'hooks.json')) remains true; reference the existing test name ('hooks.json: uninstall deletes hooks.json when GSD was its only entry') and use the same helpers runCodexInstall and runCodexUninstall so you explicitly verify the "GSD-only ⇒ unlink" behavior versus the case with an additional top-level key.
79-93: Consider importing the marker constant instead of hardcoding'gsd-check-update'.
countCodexGsdHooksduplicates theGSD_CODEX_HOOK_MARKERsubstring ('gsd-check-update') defined inbin/install.js. Importing and using the exported constant (the wayGSD_CODEX_MARKERis imported on Line 28) would keep the test and implementation in lock-step if the marker ever changes.🤖 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 79 - 93, Replace the hardcoded 'gsd-check-update' literal in countCodexGsdHooks with the exported marker constant; import GSD_CODEX_HOOK_MARKER at the top of the test module (the same way GSD_CODEX_MARKER is imported) and use it in the conditional that checks h.command.includes(...). This keeps countCodexGsdHooks in sync with the marker defined in bin/install.js and still uses readCodexHooksJson and the existing loop logic unchanged.
50-62: Minor: pre-existingpreviousCodeHometypo carried intorunCodexUninstall.The variable name
previousCodeHome(should bepreviousCodexHome) is duplicated fromrunCodexInstall. Not a functional issue, but worth normalizing both helpers in a follow-up since you’re already touching this area.🤖 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 50 - 62, The helper runCodexUninstall uses a misspelled/ inconsistent variable name previousCodeHome; rename it to previousCodexHome (update the const declaration and all references in the finally block, including the conditional that deletes/restores process.env.CODEX_HOME) so the helper matches runCodexInstall’s naming and avoids the typo; ensure you update both the declaration and the two places it’s read (the if check and the assignment/delete).tests/bug-2698-crlf-install.test.cjs (2)
142-166: Consider also asserting the new hook landed inhooks.jsonfor the mixed-EOL case.The LF and CRLF tests now verify both that the stale block is stripped from
config.tomland that the new hook is registered inhooks.json. The mixed-EOL test only asserts the strip half, leaving a small gap: a regression that brokehooks.jsonwrites specifically when the sourceconfig.tomlhad mixed line endings would slip through here. Adding the samehooks.jsonassertion (or sharing the helper from the previous comment) would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bug-2698-crlf-install.test.cjs` around lines 142 - 166, The mixed-EOL test currently only asserts the stale block was removed from config.toml; add the same hooks.json presence assertion used in the LF/CRLF tests to confirm the new hook was actually registered: after install(false, 'codex') read the .codex/hooks.json file (same path used in the other tests), parse it and assert it contains the expected hook entry (e.g. contains 'gsd-update-check' or the new hook id) — this can reuse the helper/logic from the other tests or replicate the assert pattern to ensure hooks.json was written for the mixed-EOL case; locate this change near the test function mixed-EOL config.toml... and the helper writeCodexConfigWithStaleHooks / install calls.
105-113: Optional: extract the hooks.json assertion to a helper to remove duplication.The same six-line
hooks.jsonparsing +hasNewHookcheck is repeated verbatim in the LF and CRLF tests (Lines 105–113 and 131–139). Hoisting it into a small helper (similar towriteCodexConfigWithStaleHooks) would mirror the pattern and keep the two tests focused on EOL specifics.♻️ Suggested helper
+ function assertNewHookInHooksJson(dir) { + const hooksJsonPath = path.join(dir, '.codex', 'hooks.json'); + const hooksJson = fs.existsSync(hooksJsonPath) + ? JSON.parse(fs.readFileSync(hooksJsonPath, 'utf-8')) + : null; + const hasNewHook = hooksJson && hooksJson.hooks && Array.isArray(hooksJson.hooks.SessionStart) && + hooksJson.hooks.SessionStart.some((entry) => Array.isArray(entry.hooks) && + entry.hooks.some((h) => typeof h.command === 'string' && h.command.includes('gsd-check-update'))); + assert.ok(hasNewHook, 'New gsd-check-update hook must appear in hooks.json after reinstall'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bug-2698-crlf-install.test.cjs` around lines 105 - 113, The duplicated logic that reads and asserts the new gsd-check-update hook (the hooksJsonPath + JSON.parse + hasNewHook check) should be extracted into a reusable test helper (e.g., assertHasGsdCheckUpdateHook or readAndAssertHooksJson) to remove repetition between the LF and CRLF tests; implement a function that takes the tmpDir (or the path to the .codex directory) and performs the fs.existsSync/JSON.parse and the same Array.isArray checks on hooks.SessionStart and the inner entry.hooks.some predicate, then replace the verbatim blocks in both tests with a single call to this helper (matching the pattern used by writeCodexConfigWithStaleHooks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/install.js`:
- Around line 2824-2827: The current normalization unconditionally replaces
malformed user-owned hooks payloads; instead, change the logic in the helper
around `data.hooks` and `data.hooks[eventName]` so we only create or mutate when
the shape is absent, not when it's present but invalid. Specifically: if
`data.hooks` is undefined, initialize it to {}; if `data.hooks` already exists
but is not an object or is an array, do not modify it (leave the file as-is).
Likewise, only set `data.hooks[eventName] = []` when `data.hooks[eventName]` is
strictly undefined; if it exists but is not an array, do not coerce it. Apply
these checks where the current `if (!data.hooks || typeof data.hooks !==
'object' || Array.isArray(data.hooks))` and `if
(!Array.isArray(data.hooks[eventName]))` logic appears.
- Around line 6572-6574: Replace the raw command string that builds `node
${updateCheckScript}` with a call to the existing buildHookCommand helper so
path quoting/portable-path handling is reused; specifically, when constructing
the SessionStart command before calling upsertManagedCodexHookInHooksJson, call
buildHookCommand with the resolved updateCheckScript (the same value currently
in updateCheckScript) and pass that result as the command to
upsertManagedCodexHookInHooksJson instead of the inline `node ...` string.
---
Nitpick comments:
In `@tests/bug-2698-crlf-install.test.cjs`:
- Around line 142-166: The mixed-EOL test currently only asserts the stale block
was removed from config.toml; add the same hooks.json presence assertion used in
the LF/CRLF tests to confirm the new hook was actually registered: after
install(false, 'codex') read the .codex/hooks.json file (same path used in the
other tests), parse it and assert it contains the expected hook entry (e.g.
contains 'gsd-update-check' or the new hook id) — this can reuse the
helper/logic from the other tests or replicate the assert pattern to ensure
hooks.json was written for the mixed-EOL case; locate this change near the test
function mixed-EOL config.toml... and the helper writeCodexConfigWithStaleHooks
/ install calls.
- Around line 105-113: The duplicated logic that reads and asserts the new
gsd-check-update hook (the hooksJsonPath + JSON.parse + hasNewHook check) should
be extracted into a reusable test helper (e.g., assertHasGsdCheckUpdateHook or
readAndAssertHooksJson) to remove repetition between the LF and CRLF tests;
implement a function that takes the tmpDir (or the path to the .codex directory)
and performs the fs.existsSync/JSON.parse and the same Array.isArray checks on
hooks.SessionStart and the inner entry.hooks.some predicate, then replace the
verbatim blocks in both tests with a single call to this helper (matching the
pattern used by writeCodexConfigWithStaleHooks).
In `@tests/codex-config.test.cjs`:
- Around line 1912-1918: Add a complementary negative test that ensures
hooks.json is preserved when it contains other top-level entries: after
runCodexInstall(codexHome) write or merge an extra top-level key (e.g.,
"version" or "sibling") into the hooks.json file, then call
runCodexUninstall(codexHome) and assert that fs.existsSync(path.join(codexHome,
'hooks.json')) remains true; reference the existing test name ('hooks.json:
uninstall deletes hooks.json when GSD was its only entry') and use the same
helpers runCodexInstall and runCodexUninstall so you explicitly verify the
"GSD-only ⇒ unlink" behavior versus the case with an additional top-level key.
- Around line 79-93: Replace the hardcoded 'gsd-check-update' literal in
countCodexGsdHooks with the exported marker constant; import
GSD_CODEX_HOOK_MARKER at the top of the test module (the same way
GSD_CODEX_MARKER is imported) and use it in the conditional that checks
h.command.includes(...). This keeps countCodexGsdHooks in sync with the marker
defined in bin/install.js and still uses readCodexHooksJson and the existing
loop logic unchanged.
- Around line 50-62: The helper runCodexUninstall uses a misspelled/
inconsistent variable name previousCodeHome; rename it to previousCodexHome
(update the const declaration and all references in the finally block, including
the conditional that deletes/restores process.env.CODEX_HOME) so the helper
matches runCodexInstall’s naming and avoids the typo; ensure you update both the
declaration and the two places it’s read (the if check and the
assignment/delete).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b161a06f-dcb8-4750-b333-8c2054fc65dc
📒 Files selected for processing (4)
CHANGELOG.mdbin/install.jstests/bug-2698-crlf-install.test.cjstests/codex-config.test.cjs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
- Don't normalize unsupported user `hooks` payloads. Returning early when `data.hooks` is an unsupported shape (string, array, scalar) preserves user data; only initialize when it's truly absent. Same rule for `data.hooks[eventName]` — skip when present-but-not-array rather than overwriting. - Quote the hook script path in the SessionStart command. Switched to the existing `buildHookCommand()` helper, which emits `node "<path>"` so config dirs containing spaces don't produce a broken shell command. Tests: three new e2e cases covering noncanonical `hooks` payload (string), noncanonical SessionStart slot (object), and the shell-quoted command shape. Updated the existing canonical-schema assertion to expect the quoted form. Full suite 5683 / 5683. Refs: PR gsd-build#2763 review by coderabbitai (round 2).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/codex-config.test.cjs (1)
79-93:countCodexGsdHooksdoesn't guard against an array-typeddata.hooksroot.
typeof []is'object', so an arraydata.hooksslips past the type guard on Line 81 andObject.keyswould then iterate numeric indices. The matching upsert path inbin/install.jsrejects this shape and refuses to write, so in practice this helper would just return 0 — but a defensiveArray.isArray(data.hooks)check would make the helper's intent unambiguous and align it with the implementation's own guard.♻️ Optional defensive check
- if (!data || !data.hooks || typeof data.hooks !== 'object') return 0; + if (!data || !data.hooks || typeof data.hooks !== 'object' || Array.isArray(data.hooks)) return 0;🤖 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 79 - 93, The guard in countCodexGsdHooks currently allows an array for data.hooks because typeof [] === 'object'; update the conditional in countCodexGsdHooks (which calls readCodexHooksJson) to explicitly reject arrays (use Array.isArray) so that it returns 0 when data.hooks is an array; replace the existing check (currently testing typeof data.hooks !== 'object') with a check that ensures data.hooks is a plain object (e.g., not null, typeof === 'object', and not Array.isArray(data.hooks')) so the function's behavior matches the upsert validation in bin/install.js.
🤖 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 2869-2871: The guard in bin/install.js currently accepts
data.hooks when it's an array (e.g., {"hooks":[]}); update the check in the
function that returns false to also reject array-shaped hooks by adding a test
like Array.isArray(data.hooks) and returning false if true, so replace the
condition that reads if (!data || typeof data !== 'object' || !data.hooks ||
typeof data.hooks !== 'object') with one that also tests for
Array.isArray(data.hooks) to ensure data.hooks is a plain object before
proceeding.
- Around line 6579-6588: The code currently prints a success message in the else
branch when hasEnabledCodexHooksFeature(configContent) is false even though
ensureCodexHooksFeature() may have failed to enable the feature; change the else
branch (the branch that currently logs "Configured Codex hooks (codex_hooks
feature)") to log a warning/error instead and include actionable context (e.g.,
that codex_hooks was not enabled and config.toml may have an unsupported
features root shape), referencing hasEnabledCodexHooksFeature,
ensureCodexHooksFeature, and upsertManagedCodexHookInHooksJson so maintainers
can locate the logic; do not claim success unless
hasEnabledCodexHooksFeature(...) is true and
upsertManagedCodexHookInHooksJson(...) succeeded.
---
Nitpick comments:
In `@tests/codex-config.test.cjs`:
- Around line 79-93: The guard in countCodexGsdHooks currently allows an array
for data.hooks because typeof [] === 'object'; update the conditional in
countCodexGsdHooks (which calls readCodexHooksJson) to explicitly reject arrays
(use Array.isArray) so that it returns 0 when data.hooks is an array; replace
the existing check (currently testing typeof data.hooks !== 'object') with a
check that ensures data.hooks is a plain object (e.g., not null, typeof ===
'object', and not Array.isArray(data.hooks')) so the function's behavior matches
the upsert validation in bin/install.js.
🪄 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: 6b469799-f1e9-4e3c-8cc5-13edbf3ef3e2
📒 Files selected for processing (2)
bin/install.jstests/codex-config.test.cjs
- Reject array-shaped `data.hooks` in the uninstall path. `typeof []`
is `'object'`, so a malformed `{"hooks":[]}` would slip past the
guard and let `Object.keys(data.hooks)` iterate numeric indices
against user data. Symmetric to the upsert helper's guard.
- Don't claim success when `codex_hooks` couldn't be enabled.
`ensureCodexHooksFeature` is a no-op when the user's config has an
unsupported root `features = ...` shape; the install was logging
"✓ Configured Codex hooks" anyway. Now emits a warning so the user
knows the SessionStart hook was not registered.
Tests: one new uninstall test for the array-shaped payload edge case.
The "skipped Codex hooks" warning path is exercised by the existing
`root inline-table features assignments are left untouched` test —
it already asserts the GSD hook was NOT written; the new behaviour
is the absence of the misleading success log. Full suite 5684 / 5684.
Refs: PR gsd-build#2763 review by coderabbitai (round 3).
Skipping the two round-3 nitpicks (defensive Array.isArray in test
helper, exporting GSD_CODEX_HOOK_MARKER) — pure polish, no behaviour
change.
Test names should describe the behaviour under test, not the review iteration that surfaced it. Removed '(CodeRabbit gsd-build#2763 round N)' suffixes from six tests; the actual scenarios remain identical.
|
Marking ready for human review. Brief summary of the iteration loop with @coderabbitai for transparency: Round 1 (initial review)
Round 2
Round 3
Final state:
Both deferred nitpicks are pure polish with no behavioural impact. Happy to take them in a follow-up if preferred. |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 6568-6595: The code currently writes the modified configContent
(with codex_hooks set) via fs.writeFileSync before attempting to upsert
hooks.json, which can enable codex_hooks even if
upsertManagedCodexHookInHooksJson(hooksJsonPath, 'SessionStart', command) fails;
change the flow so you do not persist the codex_hooks change until after a
successful upsert: call ensureCodexHooksFeature and
setManagedCodexHooksOwnership to compute the candidate configContent but defer
calling fs.writeFileSync(configPath, ...) until after
upsertManagedCodexHookInHooksJson returns true (only then write the updated
config), and if upsert fails keep the original config on disk (or restore the
prior content) and call hasEnabledCodexHooksFeature to decide messaging; in
short, compute but don’t persist the codex_hooks change before a successful
upsertManagedCodexHookInHooksJson, and ensure
buildHookCommand/hasEnabledCodexHooksFeature logic uses the in-memory candidate
config only for decisions until persistence is safe.
🪄 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: 39ab142d-54e1-4d33-89e0-5f269b3fa3bc
📒 Files selected for processing (2)
bin/install.jstests/codex-config.test.cjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/codex-config.test.cjs
MiaoDX
force-pushed
the
fix/2637-codex-hooks-json
branch
from
077ea55 to
c2ec7f9
Compare
… config.toml Codex 0.124.0+ rejects `[[hooks]]` in config.toml with `invalid type: map, expected a sequence`. Move the managed GSD update-check hook to ~/.codex/hooks.json (Codex's canonical location, per https://developers.openai.com/codex/hooks). Fresh install: - keep `features.codex_hooks = true` in config.toml - register SessionStart hook in hooks.json (preserve user entries, shell-quoted command path so config dirs with spaces work) - warn instead of claiming success when codex_hooks could not be enabled or when hooks.json has an unsupported root shape Migration on (re)install: - single \r?\n-aware regex strips legacy `# GSD Hooks\n[[hooks]]…` block (covers `gsd-check-update.js` and the older `gsd-update-check.js`, LF / CRLF / mixed EOLs in one pass) Hooks.json safety: - read malformed/array-rooted JSON read-only; never overwrite user-owned files we can't safely merge - preserve sibling top-level keys; only unlink the file when the whole root becomes empty - symmetric guards in upsert and uninstall paths Tests: existing `[[hooks]]` assertions moved to hooks.json; nine new e2e tests cover canonical schema, merge with pre-existing user hooks, malformed-file safety, array-rooted/unsupported user shapes, shell-quoting, sibling-key preservation, and uninstall paths. Full suite passes.
MiaoDX
force-pushed
the
fix/2637-codex-hooks-json
branch
from
c2ec7f9 to
331061e
Compare
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Adversarial review — Codex API verification + line-traced critique
Applying Gall's Law, Goodhart's Law, Kernighan's Law, Knuth's optimization principle, and Peter Principle. Every line traced. Codex API claims independently verified against developers.openai.com/codex/hooks.
Track record context
5 prior PRs to this repo: 2 merged (#1346, #1144), 3 closed without merge (#1763, #1757, #1143). That 60% rejection rate triggers elevated scrutiny — three previous attempts to fix Codex hooks have already failed in this repo (the PR body itself acknowledges #2640 and #2730 closed for sprawl). The fourth attempt deserves more skepticism, not less, because three working hypotheses about "what's wrong with Codex hooks" have already been wrong.
Codex API verification — your premise is partially overstated
You claim:
Codex 0.124.0+ rejects the inline shape with
invalid type: map, expected a sequence in 'hooks', leaving GSD-installed Codex setups unable to start.
I queried the official Codex hooks docs directly. The truth is narrower than your framing:
"In practice, the four most useful locations are:
~/.codex/hooks.json,~/.codex/config.toml,<repo>/.codex/hooks.json,<repo>/.codex/config.toml"
Inline [[hooks]] in config.toml is still supported — but the schema is [[hooks.SessionStart]] (nested table arrays keyed by event), not the flat [[hooks]]\nevent = "SessionStart" shape GSD historically wrote. Codex 0.124.0 rejects the flat-with-event-field shape, not "inline TOML hooks" generically. Your migration is correct in approach (hooks.json is canonical) but your justification for moving away from TOML conflates two different schemas.
This matters because a future contributor reading the PR body will believe "TOML hooks are broken" — they aren't. They are alive, and the docs explicitly enumerate them as one of four supported locations. The honest framing:
The flat
[[hooks]]\nevent = "SessionStart"GSD wrote is a deprecated shape Codex 0.124.0+ rejects. The fix could either (a) emit nested[[hooks.SessionStart]]in config.toml, or (b) move to canonicalhooks.json. We chose (b) because hooks.json is the documented canonical location and matches ecosystem convention (obra/superpowers, oh-my-codex). When both representations exist, Codex warns at startup — single-representation is preferred.
Please update the PR body with the precise framing.
Goodhart violations — tests measure substrings, not behavior
countCodexGsdHooks is a substring counter, not a hook validator
tests/codex-config.test.cjs:74-90:
function countCodexGsdHooks(codexHome) {
const data = readCodexHooksJson(codexHome);
if (!data || !data.hooks || typeof data.hooks !== 'object') return 0;
let count = 0;
for (const event of Object.keys(data.hooks)) {
if (!Array.isArray(data.hooks[event])) continue;
for (const entry of data.hooks[event]) {
if (!entry || !Array.isArray(entry.hooks)) continue;
for (const h of entry.hooks) {
if (h && typeof h.command === 'string' && h.command.includes(GSD_CODEX_HOOK_MARKER)) count++;
}
}
}
return count;
}This counts any hook in any event slot whose command contains the substring gsd-check-update. What passes this test while shipping a broken install:
-
Wrong event slot: A regression that puts the GSD hook into
PostToolUseinstead ofSessionStartwould still count as 1. The dedicated'hooks.json: GSD hook is registered in canonical schema'test pins the SessionStart slot, but every other test usingcountCodexGsdHooksis event-blind. If a future refactor breaks the event registration, only one of your fifteen test sites catches it. -
Broken path: A hook with
command = "node /wrong/path/gsd-check-update.js"— file doesn't exist, hook silently fails at runtime — passes this counter. The#2153test (gsd-check-update.js must exist) checks file existence but not that the path in the registered command matches. It's possible to register a stale path and have the file exist at a different location and have both tests pass. -
Substring collision: A user hook whose command happens to mention
gsd-check-update(e.g.,echo "running gsd-check-update test") counts as a GSD hook. You marked this "deferred" in your round-3 summary — but the helper exports the marker name, encouraging external tests to use the same loose check. At minimum, anchor ongsd-check-update.js(with the file extension) rather than the bare token.
Fix: rename the helper to findManagedGsdHookEntries(codexHome) and return the actual matched entries with their event slot. Then assertions become assert.strictEqual(matches.length, 1) AND assert.strictEqual(matches[0].event, 'SessionStart') — pinning both shape and slot.
'hooks.json: malformed user hooks.json is left untouched' — under-asserts
tests/codex-config.test.cjs:1786-1796:
const malformed = '{ this is not valid json';
fs.writeFileSync(userHooksPath, malformed);
runCodexInstall(codexHome);
assert.strictEqual(fs.readFileSync(userHooksPath, 'utf8'), malformed, 'malformed file preserved');Asserts the malformed file is preserved. Does not assert that config.toml was not mutated, that codex_hooks was not enabled, or that the install logged a warning rather than a success. This is the round-4 ordering bug exactly — the test passes today but doesn't lock the ordering invariant.
Round 4 unresolved — persistence ordering bug
You marked this PR "ready for human review" at 06:36:52Z. CodeRabbit's round-4 inline comment posted at 06:38:04Z (1 minute later) flagged a real bug:
"The code currently writes the modified configContent (with codex_hooks set) via fs.writeFileSync before attempting to upsert hooks.json, which can enable codex_hooks even if upsertManagedCodexHookInHooksJson(hooksJsonPath, 'SessionStart', command) fails"
You did not respond. Tracing the diff confirms the bug:
bin/install.js:6532-6595 (sequence):
fs.writeFileSync(configPath, configContent, 'utf-8'); // ← codex_hooks=true PERSISTED HERE
if (hasEnabledCodexHooksFeature(configContent)) {
const command = buildHookCommand(targetDir, 'gsd-check-update.js');
if (upsertManagedCodexHookInHooksJson(hooksJsonPath, command)) {
console.log(` ✓ Configured Codex hooks (SessionStart)`);
} else {
console.warn(` ⚠ Skipped hooks.json update — file is malformed or has an unsupported root shape`);
}
}If a user has a malformed hooks.json, this writes codex_hooks = true to config.toml, then logs a warning that hooks.json wasn't updated. Net effect: Codex starts up with the feature flag enabled but no GSD hook registered. The user sees the warning once during install, then likely never again. On the next gsd update run, the same warning fires; codex_hooks stays enabled forever.
Fix: defer fs.writeFileSync(configPath, ...) until after the upsert succeeds. If upsert fails, leave config.toml at its prior content. Alternatively, predicate the config.toml write on the upsert outcome.
Add a regression test for it: 'install does not enable codex_hooks in config.toml when hooks.json upsert fails'.
Gall's Law — partial credit
You explicitly evolved from the simpler #2640/#2730 attempts and narrowed scope (TOML escape decoding, comment-after-header parsing, malformed-file repair all deliberately deferred). That's correct application of Gall's Law to scope.
But within the PR you make two distinct changes that could have been separate:
- Migrate
[[hooks]]block out of config.toml. - Register hook in canonical hooks.json with full read/parse/merge/write flow.
You could have shipped (1) alone and let users add hook to hooks.json themselves. The 60-line upsertManagedCodexHookInHooksJson plus the 80-line removeManagedCodexHookFromHooksJson plus the matched-pair test surface (5 install + 6 uninstall scenarios) is a lot of code for a cleanup. You skipped the simpler intermediate.
This isn't disqualifying — coupled (1)+(2) is a reasonable choice if you treat hooks.json as a true contract. But the PR body should justify the choice rather than glossing it.
Kernighan's Law
The migration regex:
/(?:\r?\n|^)# GSD Hooks(?:\r?\n)\[\[hooks\]\](?:\r?\n)event = "SessionStart"(?:\r?\n)command = "node [^\r\n]*gsd-(?:check-update|update-check)\.js"(?:\r?\n)/g
Edge case I traced:
command = "node /path/to/gsd-check-update.js extra-arg" — does NOT match, because [^\r\n]*\.js requires .js to be followed by (?:\r?\n), and here it's followed by \s. Legacy installs that hand-edited the command to add args won't be migrated. Probably never happens in practice, but worth a one-line comment so a future reader knows the regex is filename-anchored, not command-anchored.
The two changed = true sites in removeManagedCodexHookFromHooksJson are partially redundant:
const filtered = data.hooks[event]
.map((entry) => {
...
if (entry.hooks.length !== before) changed = true; // ← inner: hook removed within an entry
return entry.hooks.length > 0 ? entry : null;
})
.filter(Boolean);
if (filtered.length !== data.hooks[event].length) changed = true; // ← outer: entry droppedWhen a GSD hook is the only hook in its entry, both fire. Confusing on first read. Either consolidate (track a single "dirty" flag) or comment that the redundancy is intentional.
Knuth optimization — JSON formatting churn
upsertManagedCodexHookInHooksJson and removeManagedCodexHookFromHooksJson both write:
fs.writeFileSync(hooksJsonPath, JSON.stringify(data, null, 2) + '\n');Forces 2-space indent and trailing newline. If a user authored hooks.json with tabs, 4-space indent, or no trailing newline, GSD silently reformats their file on every install. Not a bug, but a user-data churn defect: the user's editor diffs flag every install as a whitespace edit, even when the only logical change is your hook.
Fix: detect existing indentation/EOL on read, preserve on write. There's detectLineEnding in scope already (you removed its callsite in this PR). Adopt the same pattern for indent.
Peter Principle / scope-of-competence
Your domain knowledge for Codex is strong — the body cites OpenAI's docs URL, references ecosystem convention (obra/superpowers, Yeachan-Heo/oh-my-codex), and acknowledges prior failure modes. That's the opposite of drive-by.
What I'm pushing back on is the review-readiness threshold. Marking ready 1 minute before CodeRabbit's round-4 comment is normal timing noise. Not responding to it for 6+ hours after it landed, while leaving the persistence-ordering bug unresolved, is the smell. The bug is real and reachable in production. A first-pass response either fixes it or contests it; silence reads as "ship it before more findings come in."
This is a Peter-Principle-adjacent concern: domain knowledge is high, but PR-management discipline is loose. The earlier closed PRs (#2640, #2730) closed for sprawl suggest the same pattern.
Summary — what to fix before merge
Blocking:
- Defer
config.tomlwrite until afterhooks.jsonupsert succeeds; rollback on failure (round-4 bug). - Add regression test asserting
codex_hooksis NOT enabled in config.toml when hooks.json upsert fails. - Update PR body to clarify that the rejected shape is the flat
[[hooks]]\nevent = ...form, not "inline TOML hooks generically".
Should fix:
-
countCodexGsdHooksshould matchgsd-check-update.jswith file extension, not the bare token. - At least one assertion per code path that pins
event === 'SessionStart', not just the count. - Preserve user indentation/EOL in
hooks.jsonwrites. - Comment the migration regex's filename-anchored constraint.
Nit:
- Consolidate the two
changed = truesites inremoveManagedCodexHookFromHooksJson.
The technical core is solid. The hooks.json schema you implement matches the official documented shape. But three previous attempts at this same fix were closed for not-quite-right reasons, and the round-4 bug is real. Address the blocking items and this is mergeable.
Follow-up — softening the "premise overstated" critique with timeline context@MiaoDX I dug into the PR history for #2637 and the picture is more nuanced than my review reflected. Sharing what I found and asking you to close the loop with reproduction artifacts. Timeline of #2637 fixes
So three independent contributors have proposed Why I called your premise overstatedThe official Codex hooks docs explicitly list inline TOML hooks as supported, with the schema What #2747 produces is So one of two things is true:
Given that two outside contributors (#2640, #2730) reported the same regression after #2747 merged, position 2 is more likely. Which makes your PR a legitimate follow-up rather than a re-litigation of already-decided territory. What I'm asking for to close #2637 definitivelyTo merge this with confidence (and to avoid re-opening the issue a fourth time when the next Codex release ships), we need a reproduction artifact, not opinion. Specifically:
If items 2 + 3 + 4 + 5 all check out, you've demonstrated:
That's enough to update the PR body to the precise framing and merge with confidence. What to do about my other findingsThe other items in my review still stand regardless of which position is right:
The track-record / drive-by framing in the review was triggered by the maintainer's elevated-scrutiny request based on prior closure ratios. Now that I see the full timeline (you correctly read the #2640/#2730 closures and intentionally narrowed scope to avoid the same fate), that framing was harsher than warranted. The discipline you showed in scoping this PR is the opposite of drive-by behavior. Three-attempt pattern signalWorth flagging to maintainers separately: when three independent contributors all hit the same Codex error message and propose the same fix within 36 hours of each other, the underlying issue probably isn't fully resolved by the merged PR. That's a process signal more than a code signal — issue closure should require either reproduction-of-fix-on-target-version or stronger contributor consensus before auto-closing via |
trek-e
added
bug
|
Conflict notice: PRs #2807 and #2802 both address the same issue (#2637) and modify overlapping files ( |
|
closing in favor of maintainer fix. please install the RC5 as mentioned in the issue and test, provide results in a new issue if not resolved. |
2 participants
Linked issue
Fixes #2637.
What it does
Moves the managed Codex
SessionStartupdate-check hook from an inline[[hooks]]block in~/.codex/config.tomlto~/.codex/hooks.json(Codex's canonical location, per https://developers.openai.com/codex/hooks). Codex 0.124.0+ rejects the inline shape withinvalid type: map, expected a sequence in 'hooks', leaving GSD-installed Codex setups unable to start.Why hooks.json (not inline config.toml)
hooks.jsonis the documented canonical location and the de-facto ecosystem convention:hooks/hooks.json.codex/hooks.json(per README: "native.codex/hooks.jsoncoverage")[[hooks]](added by #18893)Currently GSD is the only ecosystem tool writing inline
[[hooks]], which (a) breaks 0.124.0+ and (b) collides withhooks.jsonentries from other tools.Scope (intentionally narrow)
This PR replaces the inline-write path. It does not try to handle every legacy variant — prior attempts (#2640, #2730) were closed for sprawl across TOML escape decoding, comment-after-header parsing, and aggressive malformed-file rewrites. Cleanup here is limited to one regex over the canonical block GSD itself emitted.
Install
features.codex_hooks = trueinconfig.toml(unchanged)SessionStarthook inhooks.jsonvia JSON merge (preserves user entries; idempotent on repeat installs)# GSD Hooks\n[[hooks]]…block fromconfig.tomlusing a single\r?\n-aware regex that covers bothgsd-check-update.jsand the oldergsd-update-check.jsfilenames in one pass (LF / CRLF / mixed EOLs)Uninstall
hooks.jsonhooks.jsonwhen GSD was its only entry; otherwise preserve user hooksOut of scope (deliberately not handled)
\"(rare; if a user hand-edited the block, they can hand-clean it)[[hooks]] # commentwith trailing comments (canonical GSD writes never produced them)hooks.jsonrepair — we read-only skip and leave user content untouched (avoids the rewrite-on-fallback footgun flagged in Fix Codex hook installation to use hooks.json instead of inline TOML hooks #2640)What was broken before
[[hooks]]\nevent = "SessionStart"\ncommand = "node …gsd-check-update.js"in~/.codex/config.toml--codex --globalinstall against current Codex.Verification
Tests
npm test→ 5650 / 5650 passtests/codex-config.test.cjsupdated (assertions that previously checked[[hooks]]inconfig.tomlnow checkhooks.json); 6 new e2e tests added:hooks.jsonafter installhooks.jsonwithout overwriting[[hooks]]inconfig.tomlhooks.jsonis left untouchedhooks.jsonwhen GSD was its only entrytests/bug-2698-crlf-install.test.cjs— assertions updated to look inhooks.jsonfor the new hook; legacy-block removal still verified for LF / CRLF / mixed-EOL fixturesLive Codex 0.125.0 (Linux)
Two scenarios exercised against a real
codex execwith isolatedCODEX_HOME:[features].codex_hooks = true, no[[hooks]]; hooks.json has the GSDSessionStartentry;codex execstarts cleanly (no parse error).[[hooks]]block + pre-existing userhooks.json(4 events from tmux-agent-status); after install the legacy block is gone, user hooks preserved, GSD hook appended;codex execstarts cleanly.Files
bin/install.js— install path replaced (~30 net), two helpers added (upsertManagedCodexHookInHooksJson,removeManagedCodexHookFromHooksJson), uninstall extended by 5 linestests/codex-config.test.cjs— assertion updates + 6 new teststests/bug-2698-crlf-install.test.cjs— 2 assertion updatesCHANGELOG.md— Unreleased entryChecklist
Fixes #NNNconfirmed-buglabel (yes — seeconfirmed-bugon Codex 0.124.0 hook regression can leave GSD-installed Codex setups unable to start #2637, set by maintainer triage on 2026-04-24)npm test→ 5650 / 5650)Breaking changes
None. Users who previously ran a GSD install will have their legacy
[[hooks]]block migrated automatically on the next install. User-authoredhooks.jsonentries are preserved.Notes for reviewers
migrateCodexHooksMapFormat(legacy[hooks.shell]→[[hooks]]) is no longer called by the install path. Its tests still pass against the standalone function. Removing the function and its tests is a small follow-up; left out here to keep this PR scoped.This is opened as a draft so prior-PR scope concerns can be discussed before review.
Summary by CodeRabbit
Bug Fixes
Chores
Documentation