-
Notifications
You must be signed in to change notification settings - Fork 170
fix: support Streamable HTTP MCP servers for Codex CLI (closes #1260) #1262
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
7fe6007
6a44d74
f2f6fbc
e068d11
4d7b017
2107359
1a75308
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,18 +155,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 | ||
|
|
@@ -184,6 +172,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 | ||
|
|
||
| # Update configuration using the chosen key | ||
| if not self.update_config({config_key: server_config}): | ||
| return False | ||
|
|
@@ -204,7 +196,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 = { | ||
|
|
@@ -224,11 +216,37 @@ 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
+223
to
+227
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. APM always builds a single-element Additionally, the
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_config = { | ||
| "url": (remote.get("url") or "").strip(), | ||
| "id": server_info.get("id", ""), | ||
| } | ||
|
Comment on lines
+234
to
+237
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Writing 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 | ||
|
|
@@ -595,6 +613,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. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,87 @@ 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitHub’s PAT format requires: |
||
| ], | ||
| } | ||
| 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.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() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it
1a75308