fix: Copilot adapter validates remote transport_type (#791)#812
Conversation
Mirror PR microsoft#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 microsoft#791
mvanhorn
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
danielmeppiel
added
panel-review
APM Review Panel VerdictDisposition: APPROVE (one optional follow-up; no required pre-merge actions) Per-persona findingsPython Architect: This is a routine bug-fix PR. One class diagram + one flow diagram. 1. OO / class diagramclassDiagram
direction LR
class MCPClientAdapter {
<<Abstract>>
+get_config_path() str
+update_config(updates) void
+get_current_config() dict
+configure_mcp_server(...) bool
+_infer_registry_name(package) str
+_warn_input_variables(...) void
}
class CopilotClientAdapter {
<<Adapter>>
+supports_user_scope bool
+_format_server_config(server_info, ...) dict
+_select_remote_with_url(remotes) dict
+_is_github_server(...) bool
+_resolve_variable_placeholders(...) str
}
class VSCodeClientAdapter {
<<Adapter>>
+_format_server_config(server_info, ...) tuple
+_select_remote_with_url(remotes) dict
+_select_best_package(packages) dict
}
class SimpleRegistryClient {
+find_server_by_reference(ref) dict
}
class GitHubTokenManager {
+get_token_for_purpose(purpose) str
}
MCPClientAdapter <|-- CopilotClientAdapter
MCPClientAdapter <|-- VSCodeClientAdapter
CopilotClientAdapter *-- SimpleRegistryClient
CopilotClientAdapter ..> GitHubTokenManager
class CopilotClientAdapter:::touched
classDef touched fill:#fff3b0,stroke:#d47600
note for CopilotClientAdapter "_select_remote_with_url is now\nbyte-identical in both adapters;\nnot yet on MCPClientAdapter"
2. Execution flow diagramflowchart TD
A["configure_mcp_server(server_url)\ncopilot.py:113"] --> B["registry_client.find_server_by_reference\n[NET] copilot.py:131"]
B --> C["_format_server_config(server_info)\ncopilot.py:151"]
C --> D{"_raw_stdio present?"}
D -->|Yes| E["return stdio config\ncopilot.py:175"]
D -->|No| F{"remotes list?"}
F -->|Yes| G["_select_remote_with_url(remotes)\ncopilot.py:192 [NEW]"]
G --> H{"usable remote found?"}
H -->|Yes| I["remote = first with non-empty URL"]
H -->|No| J["remote = remotes[0] fallback"]
I --> K["transport = remote.get('transport_type').strip()\ncopilot.py:199 [NEW]"]
J --> K
K --> L{"transport empty?"}
L -->|Yes| M["transport = 'http'\ncopilot.py:201 [NEW]"]
L -->|No| N{"transport in\nsse/http/streamable-http?"}
N -->|No| O["raise ValueError\ncopilot.py:203 [NEW]"]
N -->|Yes| P["build config: type='http', url=url.strip()\ncopilot.py:208 [NEW]"]
M --> P
P --> Q{"is_github_server?"}
Q -->|Yes| R["GitHubTokenManager.get_token_for_purpose\ncopilot.py:222 (unchanged)"]
Q -->|No| S["skip auth headers"]
R --> T["config headers = Bearer token (unchanged)"]
T --> U["update_config\n[FS] copilot.py:143"]
S --> U
F -->|No| V["packages path / ValueError\ncopilot.py:263+"]
Design patterns
CLI Logging Expert: No new logging regressions. The DevX UX Expert: No CLI surface changes -- no new flags, commands, or help text. From the user perspective this converts a silent failure (garbage Copilot config written with whatever transport the registry sent) into an explicit, named error. That is the correct direction: "Failure mode is the product." Error text names the transport, the server, and the supported set -- all three things a developer needs to diagnose. No blockers. Supply Chain Security Expert: The registry is an external data source. Old code silently accepted any Auth Expert: Not activated -- the PR modifies OSS Growth Hacker: Parity fix with the VS Code adapter (#656). No standalone story hook, but it contributes to the "APM validates everything from the registry" reliability narrative. CHANGELOG entry is correctly placed in CEO arbitrationAll specialists converge: this is a clean, correctly scoped parity fix from an external contributor. The Python Architect and Growth Hacker both independently flagged Required actions before mergeNone. Optional follow-ups
|
2 participants
Description
Resolves #791.
Mirrors PR #656 in the Copilot adapter. The Copilot client currently accepts any
transport_typevalue (or none) from registry remotes without validation, so a registry entry returningtransport_type: "grpc"would silently produce a garbage config instead of failing loud.After this change,
src/apm_cli/adapters/client/copilot.py:_select_remote_with_url, mirrorsvscode.py:_select_remote_with_url), falling back to the original behavior when no remote has a URL so the downstream empty-URL error path is preserved.transport_type, strips whitespace, defaults to"http"when missing/empty/whitespace-only, and raisesValueErrorfor non-empty unrecognized transports with the same message shape as the VS Code adapter:"type": "http"in the final Copilot CLI config (Copilot spec requirement for auth) and now strips the URL via.strip().The helper is a
@staticmethodonCopilotClientAdapterfor now; promoting it to a shared base (per the issue's point #3) is left for a follow-up so this PR stays scoped.Type of change
Testing
uv run pytest tests/unit/ -q→ 3715 passed)New test file
tests/unit/test_copilot_adapter.py(parallel totest_vscode_adapter.py):test_remote_missing_transport_type_defaults_to_httptest_remote_empty_transport_type_defaults_to_httptest_remote_none_transport_type_defaults_to_httptest_remote_whitespace_transport_type_defaults_to_httptest_remote_unsupported_transport_raises(asserts message includes transport name, server name, and "Copilot")test_remote_supported_transports_do_not_raise(parametrized overhttp,sse,streamable-http)test_remote_skips_entries_without_url_select_remote_with_urlTotal: 10 new tests, all passing. Full
tests/unit/suite passes unchanged.Fixes #791