Skip to content

fix mcp install for codex #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
khaliqgant merged 1 commit into from
Mar 20, 2026
Merged

fix mcp install for codex #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions packages/cli/src/__tests__/install-file-locations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,62 @@ Follow TypeScript best practices.
expect(mcpConfig.mcpServers['test-server'].command).toBe('npx');
});

it('installs MCP server to .codex/config.toml when --as codex is used (as + editor)', async () => {
// This tests the real CLI path: --as codex sets both as: 'codex' and editor: 'codex'
const mockPackage = {
id: '@test/mcp-as-codex',
name: '@test/mcp-as-codex',
format: 'mcp',
subtype: 'server',
tags: ['mcp'],
total_downloads: 5,
verified: false,
latest_version: {
version: '1.0.0',
tarball_url: 'https://example.com/package.tar.gz',
},
};

mockClient.getPackage.mockResolvedValue(mockPackage);
mockClient.downloadPackage.mockResolvedValue(await createMCPTarball(mcpServerJson));

// The CLI passes both as and editor when --as codex is used
Copy link
Copy Markdown

@codeant-ai codeant-ai Bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This test can produce false positives because .codex/config.toml is not cleaned by the shared beforeEach, so a stale config from earlier MCP Codex tests can satisfy assertions even if this install path regresses. Explicitly remove .codex at the start of this test to guarantee isolation. [logic error]

Severity Level: Major ⚠️ 
- ⚠️ Codex MCP install regressions may pass CI unnoticed.
- ⚠️ Test isolation broken in install-file-locations suite.
Suggested change
// The CLI passes both as and editor when --as codex is used
// Ensure isolation: .codex is not cleared in the shared beforeEach list
await fs.rm(path.join(testDir, '.codex'), { recursive: true, force: true });
Steps of Reproduction ✅ 
1. Run this test file's suite
(`packages/cli/src/__tests__/install-file-locations.test.ts`): `beforeEach` at line 77
clears many paths, but its `dirs` list (lines 79-30) does not include `.codex`, so
`.codex/config.toml` can persist between tests.

2. In the same `describe('MCP server packages')` block, earlier tests at lines 870 and 896
call `handleInstall(..., { editor: 'codex' })` and read `.codex/config.toml`, creating
Codex config state on disk; persistence is real because `saveFile` is mocked as
`vi.fn(actual.saveFile)` at line 41.

3. The target test at line 947 calls `handleInstall('@test/mcp-as-codex', { as: 'codex',
editor: 'codex' })` (line 967), then only checks file content from `.codex/config.toml`
(line 970) for expected strings.

4. If Codex MCP writing regresses in runtime install logic
(`packages/cli/src/commands/install.ts` MCP branch around lines 1004-1067), this test can
still pass by reading stale config from previous tests, producing a false positive CI
result.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** packages/cli/src/__tests__/install-file-locations.test.ts
**Line:** 966:966
**Comment:**
	*Logic Error: This test can produce false positives because `.codex/config.toml` is not cleaned by the shared `beforeEach`, so a stale config from earlier MCP Codex tests can satisfy assertions even if this install path regresses. Explicitly remove `.codex` at the start of this test to guarantee isolation.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

await handleInstall('@test/mcp-as-codex', { as: 'codex', editor: 'codex' });

// Should write to .codex/config.toml, NOT to AGENTS.md
const codexConfig = await fs.readFile(path.join(testDir, '.codex', 'config.toml'), 'utf-8');
expect(codexConfig).toContain('[mcp_servers.test-server]');
expect(codexConfig).toContain('command = "npx"');

// Should NOT have saved any AGENTS.md content
expect(saveFile).not.toHaveBeenCalledWith(expect.stringContaining('AGENTS.md'), expect.any(String));
});

it('installs MCP tool to .cursor/mcp.json when --as cursor is used (as + editor)', async () => {
const mockPackage = {
id: '@test/mcp-as-cursor',
name: '@test/mcp-as-cursor',
format: 'mcp',
subtype: 'tool',
tags: ['mcp'],
total_downloads: 5,
verified: false,
latest_version: {
version: '1.0.0',
tarball_url: 'https://example.com/package.tar.gz',
},
};

mockClient.getPackage.mockResolvedValue(mockPackage);
mockClient.downloadPackage.mockResolvedValue(await createMCPTarball(mcpServerJson));

await handleInstall('@test/mcp-as-cursor', { as: 'cursor', editor: 'cursor' });

const cursorConfig = JSON.parse(await fs.readFile(path.join(testDir, '.cursor', 'mcp.json'), 'utf-8'));
expect(cursorConfig.mcpServers['test-server']).toBeDefined();
expect(cursorConfig.mcpServers['test-server'].command).toBe('npx');
});

it('preserves MCP format even when auto-detection would pick agents.md', async () => {
// Create .agents.md directory to trigger auto-detection
await fs.mkdir(path.join(testDir, '.agents.md'), { recursive: true });
Expand Down
10 changes: 7 additions & 3 deletions packages/cli/src/commands/install.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,9 @@ export async function handleInstall(

// Client-side format conversion (if --as flag is specified)
// Skip conversion for snippets - they're raw content that doesn't need format conversion
if (options.as && format && format !== pkg.format && effectiveSubtype !== 'snippet') {
// Skip conversion for MCP server packages targeting an MCP editor — they use the dedicated MCP install path
const isMCPToEditor = isMCPServerPackage && MCP_EDITORS.includes(format as MCPEditor);
Copy link
Copy Markdown

@codeant-ai codeant-ai Bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: MCP editor inference is incomplete: when reinstalling from lockfile, as can be codex/cursor without editor being set, so this path is detected as MCP-to-editor but later defaults to Claude and writes MCP config to the wrong file. Infer and persist options.editor from format when format is an MCP editor to keep installs targeting the expected editor config. [logic error]

Severity Level: Critical 🚨 
- ❌ Lockfile reinstall targets wrong MCP config file.
- ❌ Codex/Cursor MCP servers may be missing after reinstall.
- ⚠️ PR's editor-specific MCP install behavior regresses.
Suggested change
const isMCPToEditor = isMCPServerPackage && MCP_EDITORS.includes(format as MCPEditor);
const inferredMcpEditor = isMCPServerPackage && MCP_EDITORS.includes(format as MCPEditor)
? (format as MCPEditor)
: undefined;
if (!options.editor && inferredMcpEditor) {
options.editor = inferredMcpEditor;
}
const isMCPToEditor = Boolean(inferredMcpEditor);
Steps of Reproduction ✅ 
1. Run lockfile reinstall path (`prpm install` with no package), which goes through
`createInstallCommand()` no-package branch at
`packages/cli/src/commands/install.ts:2071-2081` and calls `installFromLockfile({ as:
convertTo, ... })`.

2. `installFromLockfile()` calls `handleInstall()` at
`packages/cli/src/commands/install.ts:1976-1986` with `as: options.as || lockEntry.format`
but does not pass `editor`.

3. For an MCP package previously installed as Codex/Cursor, `handleInstall()` identifies
MCP path (`isMCPToEditor` logic at `install.ts:671-672`, MCP branch at
`install.ts:1006-1007`) but later resolves editor with fallback `const editor =
options.editor || 'claude'` at `install.ts:1029`.

4. MCP merge writes using Claude default path semantics
(`packages/cli/src/core/mcp.ts:338-367`), so config is written to `.mcp.json`/Claude
location instead of intended `.codex/config.toml` or `.cursor/mcp.json`.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** packages/cli/src/commands/install.ts
**Line:** 671:671
**Comment:**
	*Logic Error: MCP editor inference is incomplete: when reinstalling from lockfile, `as` can be `codex`/`cursor` without `editor` being set, so this path is detected as MCP-to-editor but later defaults to Claude and writes MCP config to the wrong file. Infer and persist `options.editor` from `format` when `format` is an MCP editor to keep installs targeting the expected editor config.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

if (options.as && format && format !== pkg.format && effectiveSubtype !== 'snippet' && !isMCPToEditor) {
console.log(` 🔄 Converting from ${pkg.format} to ${format}...`);

// Find the main file to convert
Expand Down Expand Up @@ -999,8 +1001,10 @@ export async function handleInstall(
destPath = '.claude/';
fileCount = installedFiles.length;
}
// Special handling for MCP server packages (install server configs to .mcp.json)
else if (effectiveFormat === 'mcp' && (effectiveSubtype === 'server' || effectiveSubtype === 'tool')) {
// Special handling for MCP server packages (install server configs to .mcp.json or editor config)
// Match when: native MCP format, OR source is MCP and --as targets an MCP editor (e.g., --as codex)
else if ((effectiveFormat === 'mcp' && (effectiveSubtype === 'server' || effectiveSubtype === 'tool')) ||
(isMCPServerPackage && (pkg.subtype === 'server' || pkg.subtype === 'tool') && isMCPToEditor)) {
console.log(` 🔧 Installing MCP Server...`);

// Find and parse the MCP server config file
Expand Down
Loading