fix(cli): refresh Codex hooks when MCP is already wired - #1150
Open
berthojoris wants to merge 2 commits into
Open
fix(cli): refresh Codex hooks when MCP is already wired#1150berthojoris wants to merge 2 commits into
berthojoris wants to merge 2 commits into
Conversation
`connect codex --with-hooks` no-oped for users with an existing mcp_servers.agentmemory block. Mirror Claude Code/Droid so re-runs still merge ~/.codex/hooks.json, and cover the path with adapter tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@berthojoris is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe Codex adapter now installs or refreshes global hooks when ChangesCodex hook lifecycle
Reference metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/cli/connect/codex.ts (1)
82-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the behavior comments.
These comments explain behavior that the
opts.withHooksbranch already shows. Keep the condition and remove the comments.Proposed change
- // --with-hooks is independent of MCP wiring (issue `#508` / openai/codex#16430). - // Re-run refreshes absolute script paths even when MCP is already in place. if (opts.withHooks) {As per coding guidelines, TypeScript source code must avoid comments explaining WHAT.
🤖 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 82 - 83, Remove the two behavior-explaining comments immediately above the opts.withHooks branch in the Codex connection flow, leaving the condition and implementation unchanged.Source: Coding guidelines
🤖 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 `@plugin/skills/agentmemory-agents/REFERENCE.md`:
- Line 13: Update the Codex CLI source text corresponding to the documentation
row so it matches the expected behavior in src/cli/connect/codex.ts, then run
npm run skills:gen to regenerate the documentation block and include the
generated output.
In `@test/codex-connect-hooks.test.ts`:
- Around line 172-187: Update the afterEach cleanup around HOME to mirror the
existing USERPROFILE restoration: delete process.env["HOME"] when ORIG was
initially undefined; otherwise restore ORIG. Keep the current USERPROFILE
handling and temporary home removal unchanged.
---
Nitpick comments:
In `@src/cli/connect/codex.ts`:
- Around line 82-83: Remove the two behavior-explaining comments immediately
above the opts.withHooks branch in the Codex connection flow, leaving the
condition and implementation 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: 22b0e84d-55b7-40a0-8ae2-495e55065fdb
📒 Files selected for processing (3)
plugin/skills/agentmemory-agents/REFERENCE.mdsrc/cli/connect/codex.tstest/codex-connect-hooks.test.ts
| | Claude Code | `claude-code` | Using MCP. Hooks are also available, see https://github.com/rohitg00/agentmemory#claude-code-one-block-paste-it. | | ||
| | Cline | `cline` | Using MCP via ~/.cline/mcp.json (CLI). VS Code users: add the same block via Cline Settings → MCP Servers → Edit JSON. | | ||
| | Codex CLI | `codex` | Using MCP. Hooks ship via the Codex plugin; on Codex Desktop, also pass --with-hooks to install the global hooks.json workaround for openai/codex#16430. | | ||
| | Codex CLI | `codex` | Using MCP. Hooks ship via the Codex plugin; on Codex Desktop, also pass --with-hooks to install (or refresh) the global ~/.codex/hooks.json workaround for openai/codex#16430, works even when MCP is already wired. | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Regenerate this documentation block.
Line 3 requires generated output. This row differs from src/cli/connect/codex.ts Line 69. Update the source text, run npm run skills:gen, and commit its output.
🤖 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 `@plugin/skills/agentmemory-agents/REFERENCE.md` at line 13, Update the Codex
CLI source text corresponding to the documentation row so it matches the
expected behavior in src/cli/connect/codex.ts, then run npm run skills:gen to
regenerate the documentation block and include the generated output.
Contributor
Author
|
Addressed review comments in commit
|
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
Fixes an issue where
agentmemory connect codex --with-hooksskipped installing/refreshing~/.codex/hooks.jsonifconfig.tomlwas already wired with MCP servers (already-wiredstatus).Changes
src/cli/connect/codex.ts):--with-hooksexecution is now independent of MCP wiring status, allowing users to refresh hook script paths without needing--force.test/codex-connect-hooks.test.ts): Added unit tests covering--with-hooksrefresh behavior, idempotency, custom hook preservation, and WindowsUSERPROFILEpath isolation.Test Plan
npx vitest run test/codex-connect-hooks.test.ts(14/14 passed)--dry-runoutput and--with-hooksbehavior whenconfig.tomlis already wiredSummary by CodeRabbit
New Features
--with-hooksoption now installs or refreshes the global hooks workaround, including when MCP configuration already exists.Bug Fixes
Documentation