From 954aaccd2a9d9290b56f10b739cd53d28955b3cc Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Tue, 21 Apr 2026 02:53:31 -0700 Subject: [PATCH] fix: Copilot adapter validates remote transport_type (#791) Mirror PR #656 in the Copilot adapter. Reject unrecognized remote transports (e.g. 'grpc') with a clear ValueError, default to 'http' when the registry omits transport_type, and skip remote entries without URLs. Copilot CLI still emits 'type': 'http' for every remote per its spec -- only the validation surface changes. Adds a new tests/unit/test_copilot_adapter.py suite parallel to tests/unit/test_vscode_adapter.py, covering missing/empty/whitespace/ None transport, the raise-on-unsupported path, the supported transport set, and the _select_remote_with_url helper. Fixes #791 --- CHANGELOG.md | 1 + src/apm_cli/adapters/client/copilot.py | 47 ++++++- tests/unit/test_copilot_adapter.py | 179 +++++++++++++++++++++++++ 3 files changed, 221 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_copilot_adapter.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 64ed5b606..2d9a904a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - VS Code adapter now defaults to `http` transport when `transport_type` is missing from remote registry data, matching Copilot adapter behavior (#654) +- Copilot adapter now validates remote `transport_type`, defaulting to `http` when missing/empty/whitespace and raising a clear `ValueError` for unrecognized transports, mirroring the VS Code adapter so both refuse bad registry data instead of silently writing garbage config (#791) - `apm install` no longer silently drops skills, agents, and commands when a Claude Code plugin also ships `hooks/*.json`. The package-type detection cascade now classifies plugin-shaped packages as `MARKETPLACE_PLUGIN` (which already maps hooks via the plugin synthesizer) before falling back to the hook-only classification, and emits a default-visibility `[!]` warning when a hook-only classification disagrees with the package's directory contents (#780) - Preserve custom git ports across protocols: non-default ports on `ssh://` and `https://` dependency URLs (e.g. Bitbucket Datacenter on SSH port 7999, self-hosted GitLab on HTTPS port 8443) are now captured as a first-class `port` field on `DependencyReference` and threaded through all clone URL builders. When the SSH clone fails, the HTTPS fallback reuses the same port instead of silently dropping it (#661, #731) - Detect port-like first path segment in SCP shorthand (`git@host:7999/path`) and raise an actionable error suggesting the `ssh://` URL form, instead of silently misparsing the port as part of the repository path (#784) diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 08eeadcc5..9c1077c9f 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -186,17 +186,36 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No # Check for remote endpoints first (registry-defined priority) remotes = server_info.get("remotes", []) if remotes: - # Use remote endpoint if available - remote = remotes[0] # Take the first remote - - # All remote servers use http type for proper authentication support + # Select the first remote with a non-empty URL; fall back to the + # first entry so downstream code still emits the historical empty + # URL error path when no remote is usable. + remote = self._select_remote_with_url(remotes) or remotes[0] + + # Validate transport_type from registry: default to "http" when + # missing/empty, raise ValueError for unsupported values. Mirrors + # the VS Code adapter check introduced in PR #656 so registry data + # with, e.g. transport_type="grpc" fails loudly instead of silently + # producing a garbage config. + transport = (remote.get("transport_type") or "").strip() + if not transport: + transport = "http" + elif transport not in ("sse", "http", "streamable-http"): + raise ValueError( + f"Unsupported remote transport '{transport}' for Copilot. " + f"Server: {server_info.get('name', 'unknown')}. " + f"Supported transports: http, sse, streamable-http." + ) + + # Copilot CLI writes "type": "http" for all remote servers so + # authentication flows (headers) are consistent regardless of the + # underlying transport advertised by the registry. config = { "type": "http", - "url": remote.get("url", ""), + "url": (remote.get("url") or "").strip(), "tools": ["*"], # Required by Copilot CLI specification "id": server_info.get("id", "") # Add registry UUID for conflict detection } - + # Add authentication headers for GitHub MCP server server_name = server_info.get("name", "") is_github_server = self._is_github_server(server_name, remote.get("url", "")) @@ -649,6 +668,22 @@ def _resolve_env_placeholders(self, value, resolved_env): """Legacy method for backward compatibility. Use _resolve_variable_placeholders instead.""" return self._resolve_variable_placeholders(value, resolved_env, {}) + @staticmethod + def _select_remote_with_url(remotes): + """Return the first remote entry that has a non-empty URL. + + Args: + remotes (list): Candidate remote entries from the registry. + + 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/tests/unit/test_copilot_adapter.py b/tests/unit/test_copilot_adapter.py new file mode 100644 index 000000000..440ac212a --- /dev/null +++ b/tests/unit/test_copilot_adapter.py @@ -0,0 +1,179 @@ +"""Unit tests for the Copilot client adapter transport validation (issue #791).""" + +import json +import os +import shutil +import tempfile +import unittest +from unittest.mock import MagicMock, patch + +from apm_cli.adapters.client.copilot import CopilotClientAdapter + + +class TestCopilotRemoteTransportValidation(unittest.TestCase): + """Validation of ``transport_type`` mirrors PR #656 (VS Code adapter).""" + + def setUp(self): + self.temp_dir = tempfile.mkdtemp() + self.temp_path = os.path.join(self.temp_dir, "mcp-config.json") + with open(self.temp_path, "w") as f: + json.dump({"mcpServers": {}}, f) + + self.mock_registry_patcher = patch( + "apm_cli.adapters.client.copilot.SimpleRegistryClient" + ) + self.mock_registry_class = self.mock_registry_patcher.start() + self.mock_registry_class.return_value = MagicMock() + + self.mock_integration_patcher = patch( + "apm_cli.adapters.client.copilot.RegistryIntegration" + ) + self.mock_integration_class = self.mock_integration_patcher.start() + self.mock_integration_class.return_value = MagicMock() + + self.get_path_patcher = patch( + "apm_cli.adapters.client.copilot.CopilotClientAdapter.get_config_path", + return_value=self.temp_path, + ) + self.get_path_patcher.start() + + def tearDown(self): + self.get_path_patcher.stop() + self.mock_integration_patcher.stop() + self.mock_registry_patcher.stop() + shutil.rmtree(self.temp_dir, ignore_errors=True) + + def test_remote_missing_transport_type_defaults_to_http(self): + """Remote with no transport_type produces a type=http config (issue #791).""" + adapter = CopilotClientAdapter() + + server_info = { + "id": "remote-1", + "name": "atlassian-mcp-server", + "remotes": [{"url": "https://mcp.atlassian.com/v1/mcp"}], + } + + config = adapter._format_server_config(server_info) + + self.assertEqual(config["type"], "http") + self.assertEqual(config["url"], "https://mcp.atlassian.com/v1/mcp") + + def test_remote_empty_transport_type_defaults_to_http(self): + """Empty string transport_type is treated as missing.""" + adapter = CopilotClientAdapter() + + server_info = { + "id": "remote-2", + "name": "remote-srv", + "remotes": [{"transport_type": "", "url": "https://example.com/mcp"}], + } + + config = adapter._format_server_config(server_info) + + self.assertEqual(config["type"], "http") + self.assertEqual(config["url"], "https://example.com/mcp") + + def test_remote_none_transport_type_defaults_to_http(self): + """Null transport_type is treated as missing.""" + adapter = CopilotClientAdapter() + + server_info = { + "id": "remote-3", + "name": "remote-srv", + "remotes": [{"transport_type": None, "url": "https://example.com/mcp"}], + } + + config = adapter._format_server_config(server_info) + + self.assertEqual(config["type"], "http") + + def test_remote_whitespace_transport_type_defaults_to_http(self): + """Whitespace-only transport_type is treated as missing.""" + adapter = CopilotClientAdapter() + + server_info = { + "id": "remote-4", + "name": "remote-srv", + "remotes": [{"transport_type": " ", "url": "https://example.com/mcp"}], + } + + config = adapter._format_server_config(server_info) + + self.assertEqual(config["type"], "http") + + def test_remote_unsupported_transport_raises(self): + """Unrecognized transport_type raises ValueError with server name.""" + adapter = CopilotClientAdapter() + + server_info = { + "id": "remote-5", + "name": "future-srv", + "remotes": [{"transport_type": "grpc", "url": "https://example.com/mcp"}], + } + + with self.assertRaises(ValueError) as ctx: + adapter._format_server_config(server_info) + + message = str(ctx.exception) + self.assertIn("Unsupported remote transport", message) + self.assertIn("grpc", message) + self.assertIn("future-srv", message) + self.assertIn("Copilot", message) + + def test_remote_supported_transports_do_not_raise(self): + """'sse' and 'streamable-http' transports pass validation.""" + adapter = CopilotClientAdapter() + + for transport in ("http", "sse", "streamable-http"): + server_info = { + "id": f"remote-{transport}", + "name": f"srv-{transport}", + "remotes": [{"transport_type": transport, "url": "https://example.com/mcp"}], + } + + config = adapter._format_server_config(server_info) + # Copilot CLI always emits type="http" for auth compatibility. + self.assertEqual(config["type"], "http") + self.assertEqual(config["url"], "https://example.com/mcp") + + def test_remote_skips_entries_without_url(self): + """Remotes with empty URLs are skipped; first usable remote wins.""" + adapter = CopilotClientAdapter() + + server_info = { + "id": "remote-multi", + "name": "multi-remote", + "remotes": [ + {"transport_type": "http", "url": ""}, + {"transport_type": "sse", "url": "https://good.example.com/sse"}, + ], + } + + config = adapter._format_server_config(server_info) + self.assertEqual(config["url"], "https://good.example.com/sse") + + +class TestCopilotSelectRemoteWithUrl(unittest.TestCase): + """Direct unit tests for the ``_select_remote_with_url`` helper.""" + + def test_returns_first_remote_with_url(self): + remotes = [ + {"url": ""}, + {"url": "https://example.com/a"}, + {"url": "https://example.com/b"}, + ] + self.assertEqual( + CopilotClientAdapter._select_remote_with_url(remotes)["url"], + "https://example.com/a", + ) + + def test_returns_none_when_no_url(self): + remotes = [{"url": ""}, {"url": " "}, {"url": None}] + self.assertIsNone(CopilotClientAdapter._select_remote_with_url(remotes)) + + def test_handles_empty_list(self): + self.assertIsNone(CopilotClientAdapter._select_remote_with_url([])) + + +if __name__ == "__main__": + unittest.main()