diff --git a/README.md b/README.md index 394682122..595f9315a 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/apm_cli/adapters/client/codex.py b/src/apm_cli/adapters/client/codex.py index 3c5d0985e..2f257b039 100644 --- a/src/apm_cli/adapters/client/codex.py +++ b/src/apm_cli/adapters/client/codex.py @@ -3,12 +3,13 @@ import logging import os from pathlib import Path +from urllib.parse import urlparse import toml from ...registry.client import SimpleRegistryClient from ...registry.integration import RegistryIntegration -from ...utils.console import _rich_warning +from ...utils.console import _rich_success, _rich_warning from .base import MCPClientAdapter _log = logging.getLogger(__name__) @@ -147,18 +148,6 @@ def configure_mcp_server( if server_info is None: 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 @@ -176,11 +165,18 @@ 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 + # Update configuration using the chosen key if not self.update_config({config_key: server_config}): return False - print(f"Successfully configured MCP server '{config_key}' for Codex CLI") + _rich_success( + f"Configured MCP server '{config_key}' for Codex CLI", + symbol="success", + ) return True except Exception as e: @@ -196,7 +192,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 = { @@ -216,11 +212,59 @@ 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 handling. + # Precedence on Codex when a server publishes BOTH a remote and a stdio + # package: prefer the stdio package (falls through to the packages branch + # below). The remote-only branch here handles the streamable-http path + # and rejects SSE / non-https / empty-url remotes with explicit warnings. + 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 " + "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() + if not remote_url: + _rich_warning( + f"Skipping MCP server '{server_name}' for Codex CLI: remote entry " + "has an empty url. Set `url:` to the server's streamable-http endpoint.", + symbol="warning", + ) + return None + + 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", ""), + } + 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 @@ -232,6 +276,13 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No ) if packages: + if remotes: + # Hybrid registry server: log that Codex prefers the stdio package + # over the remote endpoint so the precedence is auditable. + _log.debug( + "Codex hybrid server '%s': preferring stdio package over remote endpoint", + server_info.get("name", "unknown"), + ) # Use the first package for configuration (prioritize npm, then docker, then others) package = self._select_best_package(packages) @@ -507,6 +558,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. diff --git a/src/apm_cli/core/conflict_detector.py b/src/apm_cli/core/conflict_detector.py index 07523f255..582ddbf7f 100644 --- a/src/apm_cli/core/conflict_detector.py +++ b/src/apm_cli/core/conflict_detector.py @@ -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 diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index e46c084e7..c52ed30fa 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -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", diff --git a/tests/unit/test_conflict_detection.py b/tests/unit/test_conflict_detection.py index 9dd52a53b..9c6ccbf46 100644 --- a/tests/unit/test_conflict_detection.py +++ b/tests/unit/test_conflict_detection.py @@ -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") diff --git a/tests/unit/test_mcp_client_factory.py b/tests/unit/test_mcp_client_factory.py index d049f7a8d..a9488c132 100644 --- a/tests/unit/test_mcp_client_factory.py +++ b/tests/unit/test_mcp_client_factory.py @@ -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() @@ -274,6 +269,239 @@ 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"}, + ], + } + ], + } + 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"}) + + @patch("apm_cli.adapters.client.codex._rich_warning") + def test_format_server_config_streamable_http_rejects_empty_url(self, mock_warn): + """Empty / whitespace-only remote URLs are rejected with a clear message.""" + for empty_value in ("", " ", None): + with self.subTest(url=empty_value): + mock_warn.reset_mock() + server_info = { + "name": "broken-remote", + "id": "broken-id", + "remotes": [ + { + "url": empty_value, + "transport_type": "streamable-http", + } + ], + } + result = self.adapter._format_server_config(server_info) + self.assertIsNone(result) + mock_warn.assert_called_once() + msg = mock_warn.call_args[0][0] + self.assertIn("broken-remote", msg) + # Message must explicitly mention that the URL is empty/missing + # rather than the misleading "no scheme" wording urlparse would + # produce for an empty string. + self.assertTrue( + "empty" in msg.lower() or "missing" in msg.lower() or "no url" in msg.lower(), + f"Expected empty/missing URL wording; got: {msg!r}", + ) + + @patch("apm_cli.adapters.client.codex._rich_success") + @patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference") + def test_configure_mcp_server_streamable_http_emits_rich_success( + self, mock_find_server, mock_success + ): + """Successful streamable-HTTP registration emits a green _rich_success line.""" + mock_find_server.return_value = { + "name": "figma", + "id": "ab12cd34-0000-0000-0000-000000000000", + "remotes": [ + { + "url": "https://mcp.figma.com/mcp", + "transport_type": "streamable-http", + } + ], + } + result = self.adapter.configure_mcp_server("figma/figma") + self.assertTrue(result) + mock_success.assert_called_once() + msg = mock_success.call_args[0][0] + self.assertIn("figma", msg) + self.assertIn("Codex CLI", msg) + + @patch("apm_cli.adapters.client.codex._rich_success") + @patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference") + def test_configure_mcp_server_stdio_emits_rich_success(self, mock_find_server, mock_success): + """stdio registrations also emit _rich_success (not bare print).""" + mock_find_server.return_value = { + "id": "test-id", + "name": "test-server", + "packages": [ + { + "registry_name": "npm", + "name": "test-package", + "version": "1.0.0", + "arguments": [], + } + ], + "environment_variables": [], + } + result = self.adapter.configure_mcp_server("test-server", "my_server") + self.assertTrue(result) + mock_success.assert_called_once() + + @patch("apm_cli.adapters.client.codex._log") + @patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference") + def test_configure_mcp_server_hybrid_logs_precedence(self, mock_find_server, mock_log): + """Hybrid servers (remotes + packages) log that packages win for Codex.""" + mock_find_server.return_value = { + "id": "hybrid-server-id", + "name": "hybrid-server", + "remotes": [{"transport_type": "streamable-http", "url": "https://example.com/mcp"}], + "packages": [ + { + "registry_name": "npm", + "name": "hybrid-package", + "version": "1.0.0", + "arguments": [], + } + ], + "environment_variables": [], + } + result = self.adapter.configure_mcp_server("hybrid-server", "hybrid") + self.assertTrue(result) + # At least one debug call must mention the precedence decision. + debug_messages = [str(call) for call in mock_log.debug.call_args_list] + self.assertTrue( + any("hybrid" in m and "package" in m.lower() for m in debug_messages), + f"Expected a debug log about packages-win precedence; got: {debug_messages}", + ) + if __name__ == "__main__": unittest.main()