fix(codex): declare agentmemory MCP server as stdio#822
Conversation
|
@jinzheng8115 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds ChangesCodex adapter stdio configuration
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds explicit type = "stdio" to the Codex MCP server TOML block and introduces a Vitest suite that validates Codex adapter detection, install wiring, and backup behavior using a temporary home directory.
Changes:
- Update Codex TOML wiring block to include
type = "stdio". - Add Vitest coverage for Codex adapter
detect()andinstall()including backup creation. - Use a temp HOME/USERPROFILE to isolate filesystem writes in tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/codex-connect.test.ts | Adds integration-style tests for Codex connect adapter behavior using a temp home dir. |
| src/cli/connect/codex.ts | Updates TOML wiring block to include an explicit stdio type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| it("install() writes a stdio MCP server block for Codex", async () => { | ||
| const codexDir = join(tmpHome, ".codex"); | ||
| require("node:fs").mkdirSync(codexDir, { recursive: true }); |
|
|
||
| it("install() creates a backup when rewriting an existing Codex config", async () => { | ||
| const codexDir = join(tmpHome, ".codex"); | ||
| require("node:fs").mkdirSync(codexDir, { recursive: true }); |
| beforeEach(() => { | ||
| tmpHome = mkdtempSync(join(tmpdir(), "am-connect-codex-")); | ||
| originalHome = process.env["HOME"]; | ||
| originalUserprofile = process.env["USERPROFILE"]; | ||
| process.env["HOME"] = tmpHome; | ||
| process.env["USERPROFILE"] = tmpHome; | ||
| vi.resetModules(); | ||
| }); |
| const mod = await import( | ||
| "../src/cli/connect/codex.js?t=" + Date.now() + "-" + importCounter++ | ||
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/codex-connect.test.ts (1)
8-78: ⚡ Quick winConsider adding test coverage for the "already-wired" case.
The test suite provides focused regression coverage for the stdio config change, but doesn't test the scenario where
config.tomlalready contains[mcp_servers.agentmemory]andforce: falseis used. This case returns{ kind: "already-wired" }(lines 80-83 incodex.ts) and is an important code path for idempotent installations.📋 Suggested test case
it("install() returns already-wired when config exists and force is false", async () => { const codexDir = join(tmpHome, ".codex"); mkdirSync(codexDir, { recursive: true }); const existingBlock = `[mcp_servers.agentmemory] type = "stdio" command = "npx" args = ["-y", "`@agentmemory/mcp`"] [mcp_servers.agentmemory.env] AGENTMEMORY_URL = "http://localhost:3111" `; writeFileSync(join(codexDir, "config.toml"), existingBlock); const a = await loadAdapter(); const result = await a.install({ dryRun: false, force: false }); expect(result.kind).toBe("already-wired"); });🤖 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 `@test/codex-connect.test.ts` around lines 8 - 78, Add a test that covers the "already-wired" path by creating ~/.codex/config.toml containing an existing [mcp_servers.agentmemory] stdio block, then calling the adapter's install({ dryRun: false, force: false }) and asserting the result.kind === "already-wired"; locate the ConnectAdapter via the existing loadAdapter helper (adapter exported from codex.js) and reuse the tmpHome setup in the test file so the test mirrors the other cases that create ~/.codex and call a.install().
🤖 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 `@test/codex-connect.test.ts`:
- Line 67: The call currently uses CommonJS require to get mkdirSync; replace
require("node:fs").mkdirSync(codexDir, { recursive: true }) with the
already-imported mkdirSync(codexDir, { recursive: true }) to avoid mixing ESM
imports and CommonJS—update the invocation in the test (same pattern as the
earlier fix around line 47) so it uses the imported mkdirSync symbol and
preserves the codexDir and options.
- Line 47: Replace the CommonJS require call with the already imported ESM
symbol: remove require("node:fs").mkdirSync(...) and call the top-level imported
mkdirSync directly (e.g., mkdirSync(codexDir, { recursive: true })), ensuring
you use the existing imported mkdirSync from the file and keep the codexDir as
the first argument.
---
Nitpick comments:
In `@test/codex-connect.test.ts`:
- Around line 8-78: Add a test that covers the "already-wired" path by creating
~/.codex/config.toml containing an existing [mcp_servers.agentmemory] stdio
block, then calling the adapter's install({ dryRun: false, force: false }) and
asserting the result.kind === "already-wired"; locate the ConnectAdapter via the
existing loadAdapter helper (adapter exported from codex.js) and reuse the
tmpHome setup in the test file so the test mirrors the other cases that create
~/.codex and call a.install().
🪄 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: 68eb8ca6-15b2-49f8-ab11-c6a83ec7a513
📒 Files selected for processing (2)
src/cli/connect/codex.tstest/codex-connect.test.ts
|
|
||
| it("install() writes a stdio MCP server block for Codex", async () => { | ||
| const codexDir = join(tmpHome, ".codex"); | ||
| require("node:fs").mkdirSync(codexDir, { recursive: true }); |
There was a problem hiding this comment.
Use the imported mkdirSync instead of require().
The test file imports mkdirSync from node:fs at line 2 but then uses require("node:fs").mkdirSync(...) here. This mixes ESM and CommonJS and violates the coding guideline: "Use TypeScript and ESM only (type: module in package.json)". As per coding guidelines, ESM should be used consistently throughout the codebase.
♻️ Proposed fix
- require("node:fs").mkdirSync(codexDir, { recursive: true });
+ mkdirSync(codexDir, { recursive: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require("node:fs").mkdirSync(codexDir, { recursive: true }); | |
| mkdirSync(codexDir, { recursive: true }); |
🤖 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 `@test/codex-connect.test.ts` at line 47, Replace the CommonJS require call
with the already imported ESM symbol: remove require("node:fs").mkdirSync(...)
and call the top-level imported mkdirSync directly (e.g., mkdirSync(codexDir, {
recursive: true })), ensuring you use the existing imported mkdirSync from the
file and keep the codexDir as the first argument.
|
|
||
| it("install() creates a backup when rewriting an existing Codex config", async () => { | ||
| const codexDir = join(tmpHome, ".codex"); | ||
| require("node:fs").mkdirSync(codexDir, { recursive: true }); |
There was a problem hiding this comment.
Use the imported mkdirSync instead of require().
Same issue as line 47: the file imports mkdirSync from node:fs at line 2, but this line uses require("node:fs").mkdirSync(...). This violates the ESM-only coding guideline. As per coding guidelines, avoid mixing ESM imports with CommonJS require().
♻️ Proposed fix
- require("node:fs").mkdirSync(codexDir, { recursive: true });
+ mkdirSync(codexDir, { recursive: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require("node:fs").mkdirSync(codexDir, { recursive: true }); | |
| mkdirSync(codexDir, { recursive: true }); |
🤖 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 `@test/codex-connect.test.ts` at line 67, The call currently uses CommonJS
require to get mkdirSync; replace require("node:fs").mkdirSync(codexDir, {
recursive: true }) with the already-imported mkdirSync(codexDir, { recursive:
true }) to avoid mixing ESM imports and CommonJS—update the invocation in the
test (same pattern as the earlier fix around line 47) so it uses the imported
mkdirSync symbol and preserves the codexDir and options.
Summary
type = "stdio"to the Codex MCP block written byagentmemory connect codexWhy
In my local Codex Desktop setup, the
agentmemoryserver entry was written without an explicit transport type. Other Codex MCP entries inconfig.tomlusetype = "stdio", and adding it locally brought the config back into the expected shape for stdio-launched MCP servers.Validation
memory_saveandmemory_lesson_savework when the standalone MCP process is launched directlysrc/cli/connect/codex.tsand adds a Codex adapter test~/.codex/config.tomlto includetype = "stdio"formcp_servers.agentmemoryas a runtime workaroundI was not able to run the full upstream test suite in this environment because getting a clean local checkout of the full repo stalled repeatedly, so this PR focuses on the minimal config change plus regression coverage.
Summary by CodeRabbit
Bug Fixes
Tests