fix(codex): --with-hooks workaround for openai/codex#16430 (closes #509)#564
Merged
Conversation
… dispatch bug Codex Desktop currently does not dispatch plugin-local hooks.json even though both CodexHooks and PluginHooks feature flags are stable + default-enabled in codex-rs/features/src/lib.rs (openai/codex#16430). MCP tools still work; lifecycle observations are silently missing. Adds `agentmemory connect codex --with-hooks` which mirrors the bundled hooks.codex.json into the user-scope ~/.codex/hooks.json: - Resolves ${CLAUDE_PLUGIN_ROOT} to the absolute bundled plugin/ path (user-scope hooks don't get plugin-root injection) - Idempotent merge: previous agentmemory entries are stripped on reinstall via the resolved scripts/ path prefix; unrelated user hooks are preserved untouched - Preserves matcher fields from the bundled manifest so PreToolUse routing still works - findPluginRoot walks up from import.meta.url to locate the plugin/ dir; works for both dist/cli.mjs (bundled) and src/ (dev) layouts - Dry-run path previews both TOML and hooks.json changes Closes #509.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR implements a workaround for Codex Desktop's inability to dispatch plugin-local ChangesCodex hooks.json workaround installation
Sequence Diagram(s)(Diagrams are included in the hidden review stack artifact above.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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}
Contributor
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)
src/cli/connect/codex.ts (1)
79-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--with-hooksis skipped for already-wired Codex configs.On Line 79, the early return prevents fallback hook installation for existing users unless they also pass
--force. That breaks the intendedagentmemory connect codex --with-hooksremediation path.Proposed fix
- if (wired && !opts.force) { + if (wired && !opts.force && !opts.withHooks) { logAlreadyWired("Codex CLI", CODEX_TOML); return { kind: "already-wired", mutatedPath: CODEX_TOML }; } + + if (wired && !opts.force && opts.withHooks) { + logAlreadyWired("Codex CLI", CODEX_TOML); + const hookResult = installCodexHooks(opts); + if (hookResult.kind === "skipped") { + p.log.warn( + `Codex hooks fallback skipped: ${hookResult.reason}. MCP wiring still applied.`, + ); + } + return { kind: "installed", mutatedPath: CODEX_HOOKS }; + }Also applies to: 118-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/connect/codex.ts` around lines 79 - 82, The early return for already-wired Codex configs (the block using wired, logAlreadyWired, CODEX_TOML) prevents the --with-hooks flow from running; change the guard to only short-circuit when the user did not request hooks (e.g. if (wired && !opts.force && !opts.withHooks) { logAlreadyWired("Codex CLI", CODEX_TOML); return { kind: "already-wired", mutatedPath: CODEX_TOML }; }), and apply the same change to the similar block that spans lines 118-125 so that passing --with-hooks (or the corresponding opts['with-hooks'] flag name used in the code) allows fallback hook installation even when wired is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/connect/codex-hooks.ts`:
- Around line 98-99: isAgentmemoryEntry currently matches by raw substring which
can falsely match unrelated commands; update isAgentmemoryEntry to resolve and
normalize scriptsDir (use path.resolve) and append path.sep, then for each
handler in HookEntry inspect the command tokens (split by whitespace) and
normalize each token (path.normalize) and check tokens startWith the normalized
scriptsDir + separator (not just includes). Modify the function
isAgentmemoryEntry and its use of handler.command to perform this true
path-prefix check so only commands whose executable/path actually live under the
scriptsDir are considered agentmemory entries.
---
Outside diff comments:
In `@src/cli/connect/codex.ts`:
- Around line 79-82: The early return for already-wired Codex configs (the block
using wired, logAlreadyWired, CODEX_TOML) prevents the --with-hooks flow from
running; change the guard to only short-circuit when the user did not request
hooks (e.g. if (wired && !opts.force && !opts.withHooks) {
logAlreadyWired("Codex CLI", CODEX_TOML); return { kind: "already-wired",
mutatedPath: CODEX_TOML }; }), and apply the same change to the similar block
that spans lines 118-125 so that passing --with-hooks (or the corresponding
opts['with-hooks'] flag name used in the code) allows fallback hook installation
even when wired is true.
🪄 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
Run ID: 40f91846-61cc-4d50-9b9e-fa3758bacf01
📒 Files selected for processing (6)
README.mdsrc/cli/connect/codex-hooks.tssrc/cli/connect/codex.tssrc/cli/connect/index.tssrc/cli/connect/types.tstest/codex-connect-hooks.test.ts
Comment on lines
+98
to
+99
| function isAgentmemoryEntry(entry: HookEntry, scriptsDir: string): boolean { | ||
| return entry.hooks.some((handler) => handler.command.includes(scriptsDir)); |
Contributor
There was a problem hiding this comment.
Tighten agentmemory-entry detection to avoid accidental user-hook removal.
Line 99 matches by raw substring, so unrelated commands containing the same text can be stripped on reinstall. Match the normalized scripts path as a true path prefix (with separator) instead.
Proposed fix
function isAgentmemoryEntry(entry: HookEntry, scriptsDir: string): boolean {
- return entry.hooks.some((handler) => handler.command.includes(scriptsDir));
+ const marker = `${scriptsDir}/`;
+ return entry.hooks.some((handler) => {
+ const command = handler.command.replace(/\\/g, "/");
+ return command.includes(marker);
+ });
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/connect/codex-hooks.ts` around lines 98 - 99, isAgentmemoryEntry
currently matches by raw substring which can falsely match unrelated commands;
update isAgentmemoryEntry to resolve and normalize scriptsDir (use path.resolve)
and append path.sep, then for each handler in HookEntry inspect the command
tokens (split by whitespace) and normalize each token (path.normalize) and check
tokens startWith the normalized scriptsDir + separator (not just includes).
Modify the function isAgentmemoryEntry and its use of handler.command to perform
this true path-prefix check so only commands whose executable/path actually live
under the scriptsDir are considered agentmemory entries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #509.
Codex Desktop currently does not dispatch plugin-local
hooks.jsoneven though bothCodexHooksandPluginHooksfeature flags are stable + default-enabled incodex-rs/features/src/lib.rs. Upstream tracking issue: openai/codex#16430. MCP tools still work; lifecycle observations are silently missing.Adds an opt-in workaround:
This mirrors the bundled
plugin/hooks/hooks.codex.jsoninto the user-scope~/.codex/hooks.json, which Codex loads reliably today.How it works
${CLAUDE_PLUGIN_ROOT}to the absolute bundledplugin/path. Codex only injects that env var for plugin-scope hooks, so user-scope entries need real paths.<pluginRoot>/scripts/prefix; unrelated user hooks are preserved untouched.matcherfields from the bundled manifest soPreToolUseevent routing keeps working (Edit|Write|Read|Glob|Grep).findPluginRootwalks up fromimport.meta.urlto locate theplugin/dir; works for bothdist/cli.mjs(bundled) andsrc/(dev) layouts.--dry-run --with-hooks) previews both TOML and hooks.json changes without mutating either file.~/.codex/hooks.jsonto~/.agentmemory/backups/before write (same pattern as the TOML wiring).Validation
test/codex-connect-hooks.test.tscover path rewrite, matcher preservation, event coverage, user-hook preservation across (re)installs, JSON round-tripnpm test: 97/97 test files pass, 1081/1081 tests passnpm run build: dist bundles cleannode dist/cli.mjs connect codex --dry-run --with-hooksreports both TOML and hooks.json previewsManual flow on Codex Desktop after merge
When upstream lands the plugin hook dispatch fix, users can either keep the user-scope mirror (still correct, just redundant) or remove the agentmemory entries from
~/.codex/hooks.jsonmanually.Scope
withHooks(no-op)Summary by CodeRabbit
New Features
--with-hooksflag to theagentmemory connect codexcommand. When enabled, it writes to~/.codex/hooks.jsonwith plugin hook handlers to address a known Codex Desktop plugin issue.Documentation
--with-hooksworkaround.