Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 0 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ Or add an MCP server (wired into Copilot, Claude, Cursor, Codex, OpenCode, Gemin
apm install --mcp io.github.github/github-mcp-server --transport http # connects over HTTPS
```

> *Codex CLI currently does not support remote MCP servers; the install will skip Codex with a notice. Omit `--transport http` to use the local Docker variant on Codex (requires `GITHUB_PERSONAL_ACCESS_TOKEN`).*
See the **[Getting Started guide](https://microsoft.github.io/apm/getting-started/quick-start/)** for the full walkthrough.

## Works with agentrc
Expand Down
77 changes: 60 additions & 17 deletions src/apm_cli/adapters/client/codex.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
import os
from pathlib import Path
from urllib.parse import urlparse

import toml

Expand Down Expand Up @@ -155,18 +156,6 @@ def configure_mcp_server(
print(f"Error: MCP server '{server_url}' not found in registry")
return False

# Check for remote servers early - Codex doesn't support remote/SSE servers
remotes = server_info.get("remotes", [])
packages = server_info.get("packages", [])

# If server has only remote endpoints and no packages, it's a remote-only server
if remotes and not packages:
print(f"[!] Warning: MCP server '{server_url}' is a remote server (SSE type)")
print(" Codex CLI only supports local servers with command/args configuration")
print(" Remote servers are not supported by Codex CLI")
print(" Skipping installation for Codex CLI")
return False

# Determine the server name for configuration key
if server_name:
# Use explicitly provided server name
Expand All @@ -184,6 +173,10 @@ def configure_mcp_server(
# Generate server configuration with environment variable resolution
server_config = self._format_server_config(server_info, env_overrides, runtime_vars)

# Skip if formatter signaled "unsupported" (e.g. SSE remote on Codex)
if server_config is None:
return False
Comment on lines 173 to +178
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fix it
1a75308


# Update configuration using the chosen key
if not self.update_config({config_key: server_config}):
return False
Expand All @@ -204,7 +197,7 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No
runtime_vars (dict, optional): Runtime variable values.

Returns:
dict: Formatted server configuration for Codex CLI.
dict | None: Formatted server configuration for Codex CLI, or None if unsupported (e.g. SSE remote).
"""
# Default configuration structure with registry ID for conflict detection
config = {
Expand All @@ -224,11 +217,48 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No
self._warn_input_variables(raw["env"], server_info.get("name", ""), "Codex CLI")
return config

# Note: Remote servers (SSE type) are handled in configure_mcp_server and rejected early
# This method only handles local servers with packages

# Get packages from server info
# Remote MCP: SSE is unsupported by Codex -- return None to skip.
remotes = server_info.get("remotes", [])
packages = server_info.get("packages", [])
if remotes and not packages:
remote = self._select_remote_with_url(remotes) or remotes[0]
server_name = server_info.get("name", "")
if (remote.get("transport_type") or "").strip() == "sse":
_rich_warning(
f"Skipping MCP server '{server_name}' for Codex CLI: SSE transport "
Comment on lines +224 to +228
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

APM always builds a single-element remotes list (mcp_integrator.py:216: info["remotes"] = [remote]), so the proposed “first SSE, later streamable-http” scenario cannot occur.

Additionally, the _select_remote_with_url(remotes) or remotes[0] pattern is already identical across the other adapters:

  • copilot.py:541
  • gemini.py:131
  • vscode.py:362

The recommendation to “mirror the other adapters’ validation” is therefore self-defeating, because the referenced implementations use the exact same logic.

"is deprecated by the MCP spec and not supported by Codex. "
"Switch to `transport: streamable-http`.",
symbol="warning",
)
return None

remote_url = (remote.get("url") or "").strip()

scheme = urlparse(remote_url).scheme.lower()
if scheme != "https":
_rich_warning(
f"Skipping MCP server '{server_name}' for Codex CLI: remote URL "
f"must use https:// (got {scheme or 'no scheme'}).",
symbol="warning",
)
return None

remote_config = {
"url": remote_url,
"id": server_info.get("id", ""),
}
Comment on lines +246 to +249
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

codex.py:617-627’s _select_remote_with_url contractually never returns a remote with an empty URL.

Writing id as an empty string is intentional and consistent with copilot.py:565. This follows the conflict_detector schema convention (introduced in commit 6a44d74).

Omitting the key would break conflict detection behavior.

http_headers: dict[str, str] = {}
for header in remote.get("headers", []):
h_name = header.get("name", "")
h_value = header.get("value", "")
if h_name and h_value:
http_headers[h_name] = self._resolve_variable_placeholders(
h_value, env_overrides or {}, runtime_vars or {}
)
if http_headers:
remote_config["http_headers"] = http_headers
self._warn_input_variables(http_headers, server_name, "Codex CLI")
return remote_config

if not packages:
# If no packages are available, this indicates incomplete server configuration
Expand Down Expand Up @@ -595,6 +625,19 @@ def _inject_docker_env_vars(self, args, env_vars):

return result

@staticmethod
def _select_remote_with_url(remotes):
"""Return the first remote entry that has a non-empty URL.

Returns:
dict or None: The first usable remote, or None if none qualify.
"""
for remote in remotes:
url = (remote.get("url") or "").strip()
if url:
return remote
return None

def _select_best_package(self, packages):
"""Select the best package for installation from available packages.

Expand Down
4 changes: 3 additions & 1 deletion src/apm_cli/core/conflict_detector.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ def get_existing_server_configs(self) -> dict[str, Any]:
server_name = raw_key[len("mcp_servers.") :]
if server_name.startswith('"') and server_name.endswith('"'):
server_name = server_name[1:-1]
if isinstance(value, dict) and ("command" in value or "args" in value):
if isinstance(value, dict) and (
"command" in value or "args" in value or "url" in value
):
servers[server_name] = value

return servers
Expand Down
9 changes: 9 additions & 0 deletions tests/unit/integration/test_mcp_integrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,15 @@ def test_sse_transport_builds_remote(self):
info = MCPIntegrator._build_self_defined_info(dep)
assert "remotes" in info

def test_streamable_http_transport_builds_remote(self):
dep = _make_self_defined(
"stream-svc", transport="streamable-http", url="https://example.com/mcp"
)
info = MCPIntegrator._build_self_defined_info(dep)
assert info["remotes"][0]["transport_type"] == "streamable-http"
assert info["remotes"][0]["url"] == "https://example.com/mcp"
assert "packages" not in info

def test_http_with_headers(self):
dep = _make_self_defined(
"headered-svc",
Expand Down
16 changes: 16 additions & 0 deletions tests/unit/test_conflict_detection.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,3 +295,19 @@ def test_codex_flat_keys_combine_with_nested_table(self):
)
configs = detector.get_existing_server_configs()
self.assertEqual(set(configs), {"nested", "flat", "quoted-name"})

def test_codex_flat_keys_detect_remote_url_entries(self):
"""Codex flat-key remote entries (url-only) are picked up alongside stdio ones."""
detector = self._make_detector(
"codex",
"mcp_servers",
{
"mcp_servers.foo": {
"url": "https://mcp.example.com/mcp",
"id": "ab12cd34-0000-0000-0000-000000000000",
},
},
)
configs = detector.get_existing_server_configs()
self.assertIn("foo", configs)
self.assertEqual(configs["foo"]["url"], "https://mcp.example.com/mcp")
158 changes: 143 additions & 15 deletions tests/unit/test_mcp_client_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,33 +159,28 @@ def test_configure_mcp_server_basic(self, mock_find_server):
server_config = config["mcp_servers"]["my_server"]
self.assertEqual(server_config["command"], "npx")

@patch("apm_cli.adapters.client.codex._rich_warning")
@patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference")
def test_configure_mcp_server_remote_rejected(self, mock_find_server):
"""Test that remote servers (SSE type) are rejected by Codex adapter."""
# Mock registry response for remote-only server
def test_configure_mcp_server_sse_remote_rejected(self, mock_find_server, mock_warn):
"""SSE remotes are rejected with a warning that points to streamable-http."""
mock_server_info = {
"id": "remote-server-id",
"name": "remote-server",
"remotes": [{"transport_type": "sse", "url": "https://example.com/mcp"}],
"packages": [], # No packages, only remote endpoints
"packages": [],
}
mock_find_server.return_value = mock_server_info

# Capture printed output
with patch("builtins.print") as mock_print:
result = self.adapter.configure_mcp_server("remote-server")
result = self.adapter.configure_mcp_server("remote-server")

# Should return False (rejected)
self.assertFalse(result)
mock_find_server.assert_called_once_with("remote-server")

# Verify warning message was printed
mock_print.assert_any_call(
"[!] Warning: MCP server 'remote-server' is a remote server (SSE type)"
)
mock_print.assert_any_call(
" Codex CLI only supports local servers with command/args configuration"
)
mock_warn.assert_called_once()
warn_message = mock_warn.call_args[0][0]
self.assertIn("remote-server", warn_message)
self.assertIn("SSE", warn_message)
self.assertIn("streamable-http", warn_message)

# Verify no config was updated
config = self.adapter.get_current_config()
Expand Down Expand Up @@ -274,6 +269,139 @@ def test_self_defined_stdio_normalizes_project_placeholders(self):
["-y", "@modelcontextprotocol/server-filesystem", ".", "."],
)

def test_format_server_config_streamable_http_writes_url_and_id(self):
"""Streamable-HTTP remote produces url + id (no http_headers when none)."""
server_info = {
"name": "figma",
"id": "ab12cd34-0000-0000-0000-000000000000",
"remotes": [
{
"url": "https://mcp.figma.com/mcp",
"transport_type": "streamable-http",
}
],
}
config = self.adapter._format_server_config(server_info)
self.assertEqual(config["url"], "https://mcp.figma.com/mcp")
self.assertEqual(config["id"], "ab12cd34-0000-0000-0000-000000000000")
self.assertNotIn("http_headers", config)

def test_format_server_config_streamable_http_writes_headers(self):
"""Registry-supplied headers land under ``http_headers``."""
server_info = {
"name": "figma",
"remotes": [
{
"url": "https://mcp.figma.com/mcp",
"transport_type": "streamable-http",
"headers": [
{"name": "Authorization", "value": "Bearer ghp_xxx"},
{"name": "X-Figma-Region", "value": "us-east-1"},
],
}
Comment on lines +297 to +301
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

GitHub’s PAT format requires:

ghp_ + 36 alphanumeric characters

],
}
config = self.adapter._format_server_config(server_info)
self.assertEqual(
config["http_headers"],
{
"Authorization": "Bearer ghp_xxx",
"X-Figma-Region": "us-east-1",
},
)

def test_format_server_config_streamable_http_self_defined(self):
"""Self-defined streamable-http info produces a remote config."""
server_info = {
"name": "my-remote",
"remotes": [
{
"transport_type": "streamable-http",
"url": "https://example.com/mcp",
"headers": [{"name": "Authorization", "value": "Bearer xyz"}],
}
],
}
config = self.adapter._format_server_config(server_info)
self.assertEqual(config["url"], "https://example.com/mcp")
self.assertEqual(config["http_headers"], {"Authorization": "Bearer xyz"})

@patch("apm_cli.adapters.client.codex._rich_warning")
def test_format_server_config_streamable_http_rejects_non_https(self, mock_warn):
"""Non-HTTPS remote URLs are rejected to prevent cleartext header leakage."""
server_info = {
"name": "evil-remote",
"id": "evil-id",
"remotes": [
{
"url": "http://mcp.example.com/mcp",
"transport_type": "streamable-http",
"headers": [{"name": "Authorization", "value": "Bearer secret"}],
}
],
}

result = self.adapter._format_server_config(server_info)

self.assertIsNone(result)
mock_warn.assert_called_once()
warn_message = mock_warn.call_args[0][0]
self.assertIn("evil-remote", warn_message)
self.assertIn("https://", warn_message)

@patch("apm_cli.adapters.client.codex._rich_warning")
@patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference")
def test_configure_mcp_server_http_remote_rejected(self, mock_find_server, mock_warn):
"""End-to-end: an http:// remote URL never lands in the Codex config."""
mock_find_server.return_value = {
"name": "evil-remote",
"id": "evil-id",
"remotes": [
{
"url": "http://mcp.example.com/mcp",
"transport_type": "streamable-http",
"headers": [{"name": "Authorization", "value": "Bearer secret"}],
}
],
"packages": [],
}

result = self.adapter.configure_mcp_server("evil-remote")

self.assertFalse(result)
mock_warn.assert_called_once()
warn_message = mock_warn.call_args[0][0]
self.assertIn("evil-remote", warn_message)
self.assertIn("https://", warn_message)

# Verify no config was written
config = self.adapter.get_current_config()
self.assertNotIn("mcp_servers", config)

@patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference")
def test_configure_mcp_server_streamable_http_writes_toml_entry(self, mock_find_server):
"""End-to-end install of a streamable-HTTP server writes a parseable TOML entry."""
mock_find_server.return_value = {
"name": "figma",
"id": "ab12cd34-0000-0000-0000-000000000000",
"remotes": [
{
"url": "https://mcp.figma.com/mcp",
"transport_type": "streamable-http",
"headers": [{"name": "Authorization", "value": "Bearer ghp_xxx"}],
}
],
}

result = self.adapter.configure_mcp_server("figma/figma")

self.assertTrue(result)
config = self.adapter.get_current_config()
figma = config["mcp_servers"]["figma"]
self.assertEqual(figma["url"], "https://mcp.figma.com/mcp")
self.assertEqual(figma["id"], "ab12cd34-0000-0000-0000-000000000000")
self.assertEqual(figma["http_headers"], {"Authorization": "Bearer ghp_xxx"})


if __name__ == "__main__":
unittest.main()