feat: convert hooks to .codex/hooks.json for Codex target#742
feat: convert hooks to .codex/hooks.json for Codex target#742nkolly wants to merge 3 commits intoEveryInc:mainfrom
Conversation
Codex supports hooks natively via .codex/hooks.json in the same format as Claude Code. The converter was silently dropping hooks for Codex while other targets (OpenCode) already converted them. Changes: - Add hooks field to CodexBundle type - Pass plugin.hooks through in claude-to-codex converter (both modes) - Write hooks to .codex/hooks.json in writeCodexBundle - Merge with existing hooks (preserves hooks from other plugins) - Tag managed hooks with _source field for clean re-installs All 58 existing Codex tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 360ded2c3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Address review feedback: always run the merge even when bundle.hooks is empty, so previously installed hooks tagged with _source are removed on upgrade. Prevents stale hooks from persisting indefinitely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tmchow
left a comment
There was a problem hiding this comment.
Future-proofing the converter so a hooks-aware plugin "just works" is reasonable, but a few things should land before merge so that day-one isn't the first time the new code paths run for real.
1. No fixture coverage for any of the new code
The new module adds three non-trivial behaviors — pass-through, source-tagged merge, and cleanup-on-empty — and the test plan is "existing tests pass." That's a regression check, not validation. The repo already has a rich hooks fixture at tests/fixtures/sample-plugin/hooks/hooks.json exercised by tests/converter.test.ts:221, but tests/codex-converter.test.ts:42 hard-codes hooks: undefined, so none of this PR's logic is touched by a test. Worth adding cases for at minimum:
- Plugin with hooks →
.codex/hooks.jsonwritten with_sourcetags - Existing
hooks.jsonfrom another_source→ preserved on install - Plugin re-install with same hooks → idempotent (see #3)
- Plugin re-install with hooks removed → managed entries cleaned, others preserved
- Existing untagged hook entries → preserved (the "manual config" case the comment promises)
2. The _source default of "coco" is a magic string
mergeCodexHooks falls back to "coco" when pluginName is undefined, and pluginName can be undefined — writeCodexBundle sets it from bundle.pluginName with no required guarantee. "coco" doesn't appear anywhere else in the repo and won't match either shipped plugin's name. If two unnamed bundles ever install in sequence they'll silently overwrite each other's hooks under the same _source tag, and a hook tagged coco is opaque to anyone inspecting hooks.json.
Either:
- Make
pluginNamerequired for the hooks write path and skip the merge entirely when it's missing, or - Throw if
pluginNameis missing — the "no source" case shouldn't silently land in_source-tagged data.
3. Idempotent re-install creates a new .bak on every run
backupFile writes hooks.json.bak.<timestamp> every time existingHooks !== null, even when the merged output is byte-identical to what's on disk. A user who runs install --to codex repeatedly will accumulate timestamped backups in .codex/ for no reason. Easy fix: only back up when the serialized merged content differs from the existing file, and only write the file in that case (also avoids the unnecessary write).
4. _source is structural data inside hook entries
Adding _source directly into the hook object means it travels into Codex's runtime alongside type/command/prompt/agent/timeout. Codex appears to ignore unknown fields today, but this couples our merge bookkeeping to whatever schema validation Codex adds in the future. Consider hoisting the bookkeeping above the entries — e.g., a sibling _managed: { <source>: [<event>: [<index>] ...] } block, or per-event grouping — so hook objects stay schema-clean. Not blocking, but worth thinking about before this is the established format.
Summary
.codex/hooks.jsonin the same format as Claude Codeclaude-to-codex.tsand writes them to.codex/hooks.jsonin the Codex writerChanges
src/types/codex.ts: Addhooks?: ClaudeHooksfield toCodexBundlesrc/converters/claude-to-codex.ts: Passplugin.hooksthrough in both agents-only and full modesrc/targets/codex.ts: Write hooks to.codex/hooks.jsonwith merge support — preserves existing hooks from other plugins via_sourcetaggingHow it works
_sourcetag are replaced on re-install.codex/hooks.jsonviaomx setup)Test plan
bun test --filter "codex")🤖 Generated with Claude Code