Skip to content

Commit ba6d333

Browse files
committed
fix(windows): platform abstraction + SDK constraint tightening
BREAKING CHANGE: SDK minimum version raised from 0.1.0 to 0.1.30 Fixes two P0 Windows bugs: - CLI binary detection: 'copilot' vs 'copilot.exe' - SDK built-in tool override: overridesBuiltInTool flag Changes: - NEW: _platform.py - Single Source of Truth for platform detection - NEW: scripts/smoke_test.py - Developer smoke test - NEW: test_platform.py, test_client_error_recovery.py, test_provider_streaming_edge.py - FIX: pyproject.toml SDK constraint >=0.1.30 (required for overridesBuiltInTool) - FIX: Remove dead shutil import from __init__.py - FIX: Windows encoding in test_user_facing_strings.py (UTF-8) - DOC: Implementation note added to spec docs Test Results: - Windows: 871 passed, 12 skipped, 0 failed - WSL: 858 passed, 5 skipped, 0 failed - SDK Assumption Tests: 100/100 both platforms - Coverage: 96.24% (new code: 100%) Skipped tests are platform-appropriate: - 12 skipped on Windows (Unix chmod/symlink operations) - 5 skipped on WSL (Windows-specific tests) Reviewed-by: Amplifier (8 specialist agents)
1 parent f2c1e39 commit ba6d333

14 files changed

Lines changed: 2678 additions & 55 deletions

Makefile

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
# Usage:
77
# make install - Install dev dependencies
88
# make test - Run all tests
9+
# make smoke - Quick E2E smoke test (seconds, not minutes)
910
# make coverage - Run tests with coverage report
1011
# make lint - Check code style
1112
# make format - Auto-format code
@@ -14,7 +15,7 @@
1415
#
1516
# =============================================================================
1617

17-
.PHONY: install test coverage lint format check clean help sdk-assumptions
18+
.PHONY: install test smoke coverage lint format check clean help sdk-assumptions
1819

1920
# Default Python - override with: make test PYTHON=python3.12
2021
PYTHON ?= python
@@ -51,6 +52,11 @@ test:
5152
sdk-assumptions:
5253
$(PYTHON) -m pytest tests/sdk_assumptions/ -v --tb=long
5354

55+
# Quick smoke test - validates provider works E2E in seconds
56+
# Use after code changes, SDK upgrades, or to debug cross-platform issues
57+
smoke:
58+
$(PYTHON) scripts/smoke_test.py --verbose
59+
5460
# Run tests with coverage
5561
coverage:
5662
$(PYTHON) -m pytest --cov=$(PACKAGE) --cov-report=term-missing --cov-report=html tests/

amplifier_module_provider_github_copilot/__init__.py

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242

4343
import asyncio
4444
import logging
45-
import shutil
4645
from collections.abc import Awaitable, Callable
4746
from typing import Any
4847

@@ -257,9 +256,11 @@ async def mount(
257256
cli_path = _find_copilot_cli(config)
258257

259258
if not cli_path:
259+
from ._platform import get_cli_binary_name
260+
260261
logger.warning(
261262
"[MOUNT] Copilot SDK prerequisites not met - provider not mounted. "
262-
"Ensure 'copilot' CLI is installed and authenticated."
263+
f"Ensure '{get_cli_binary_name()}' CLI is installed and authenticated."
263264
)
264265
return None # Graceful degradation
265266

@@ -334,32 +335,19 @@ def _find_copilot_cli(config: dict[str, Any]) -> str | None:
334335
335336
Returns:
336337
Resolved CLI path, or None if not found
338+
339+
Note:
340+
Platform-specific binary naming (copilot vs copilot.exe) is handled
341+
by the _platform module to ensure single source of truth.
337342
"""
338343
try:
339-
try:
340-
from pathlib import Path
341-
342-
import copilot as _copilot_mod # type: ignore[import-untyped]
343-
344-
_mod_file = _copilot_mod.__file__
345-
if _mod_file is None:
346-
raise ImportError("copilot module has no __file__")
347-
_cli_bin = Path(_mod_file).parent / "bin" / "copilot"
348-
if _cli_bin.exists():
349-
cli_path = str(_cli_bin)
350-
_ensure_executable(cli_path)
351-
logger.debug(f"[MOUNT] Found SDK bundled CLI at: {cli_path}")
352-
return cli_path
353-
else:
354-
logger.debug("[MOUNT] SDK bundled CLI binary not found on disk")
355-
except ImportError:
356-
logger.debug("[MOUNT] copilot SDK not installed, trying PATH")
357-
358-
found = shutil.which("copilot") or shutil.which("copilot.exe")
359-
if found:
360-
_ensure_executable(found)
361-
logger.debug(f"[MOUNT] Found Copilot CLI in PATH at: {found}")
362-
return found
344+
from ._platform import locate_cli_binary
345+
346+
cli_path = locate_cli_binary()
347+
if cli_path:
348+
_ensure_executable(str(cli_path))
349+
logger.debug(f"[MOUNT] Found Copilot CLI at: {cli_path}")
350+
return str(cli_path)
363351

364352
logger.debug("[MOUNT] Copilot CLI not found via SDK or PATH")
365353
return None
Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,280 @@
1+
"""Cross-platform utilities for the GitHub Copilot provider module.
2+
3+
This module centralizes ALL platform-specific logic following the principle
4+
of Single Source of Truth. Platform detection and binary naming are done
5+
in ONE place to prevent the scattered `sys.platform` checks that led to
6+
the Windows .exe bug (Hotfix 2026-03-07).
7+
8+
Design Patterns Used:
9+
- Strategy Pattern: Platform-specific behavior via PlatformInfo
10+
- Factory Function: get_platform_info() returns appropriate implementation
11+
- Single Source of Truth: Binary names defined once, used everywhere
12+
13+
Cross-Platform Considerations:
14+
- Windows: Uses .exe extension for executables
15+
- Linux/macOS/WSL: No extension for executables
16+
- WSL: Reports as 'linux' (sys.platform), uses Unix conventions
17+
- Cygwin: Reports as 'cygwin', uses Unix conventions
18+
19+
Usage:
20+
from ._platform import get_cli_binary_name, get_sdk_binary_path
21+
22+
# Get just the binary name for current platform
23+
name = get_cli_binary_name() # "copilot.exe" on Windows, "copilot" elsewhere
24+
25+
# Get full path from SDK module
26+
path = get_sdk_binary_path() # Full path or None
27+
"""
28+
29+
from __future__ import annotations
30+
31+
import logging
32+
import shutil
33+
import sys
34+
from dataclasses import dataclass
35+
from functools import lru_cache
36+
from pathlib import Path
37+
38+
logger = logging.getLogger(__name__)
39+
40+
# ═══════════════════════════════════════════════════════════════════════════════
41+
# Constants - Single Source of Truth for binary names
42+
# ═══════════════════════════════════════════════════════════════════════════════
43+
44+
CLI_BINARY_NAME_UNIX = "copilot"
45+
CLI_BINARY_NAME_WINDOWS = "copilot.exe"
46+
CLI_BINARY_SUBDIR = "bin"
47+
48+
49+
# ═══════════════════════════════════════════════════════════════════════════════
50+
# Platform Detection
51+
# ═══════════════════════════════════════════════════════════════════════════════
52+
53+
54+
@dataclass(frozen=True)
55+
class PlatformInfo:
56+
"""Immutable platform information.
57+
58+
This dataclass encapsulates all platform-specific details needed
59+
for CLI binary location. Using frozen=True makes it immutable,
60+
preventing accidental modification.
61+
62+
Attributes:
63+
name: Human-readable platform name (for logging)
64+
is_windows: True if running on Windows
65+
cli_binary_name: Name of the CLI binary ("copilot" or "copilot.exe")
66+
uses_exe_extension: True if platform uses .exe for executables
67+
"""
68+
69+
name: str
70+
is_windows: bool
71+
cli_binary_name: str
72+
uses_exe_extension: bool
73+
74+
75+
@lru_cache(maxsize=1)
76+
def get_platform_info() -> PlatformInfo:
77+
"""Get platform information for the current system.
78+
79+
This is the ONLY place where sys.platform is checked for binary naming.
80+
All other code should use this function.
81+
82+
The result is cached since platform doesn't change during runtime.
83+
Use get_platform_info.cache_clear() to reset if needed (e.g., testing).
84+
85+
Returns:
86+
PlatformInfo with current platform details
87+
"""
88+
is_windows = sys.platform == "win32"
89+
90+
if is_windows:
91+
return PlatformInfo(
92+
name="Windows",
93+
is_windows=True,
94+
cli_binary_name=CLI_BINARY_NAME_WINDOWS,
95+
uses_exe_extension=True,
96+
)
97+
elif sys.platform == "darwin":
98+
return PlatformInfo(
99+
name="macOS",
100+
is_windows=False,
101+
cli_binary_name=CLI_BINARY_NAME_UNIX,
102+
uses_exe_extension=False,
103+
)
104+
else:
105+
# Linux, WSL, Cygwin, FreeBSD, etc.
106+
return PlatformInfo(
107+
name="Unix",
108+
is_windows=False,
109+
cli_binary_name=CLI_BINARY_NAME_UNIX,
110+
uses_exe_extension=False,
111+
)
112+
113+
114+
# ═══════════════════════════════════════════════════════════════════════════════
115+
# Binary Location Functions
116+
# ═══════════════════════════════════════════════════════════════════════════════
117+
118+
119+
def get_cli_binary_name() -> str:
120+
"""Get the CLI binary name for the current platform.
121+
122+
Returns:
123+
"copilot.exe" on Windows, "copilot" elsewhere
124+
"""
125+
return get_platform_info().cli_binary_name
126+
127+
128+
def get_sdk_binary_path() -> Path | None:
129+
"""Get the path to the SDK-bundled CLI binary.
130+
131+
Attempts to locate the CLI binary bundled with the copilot SDK package.
132+
This is the preferred binary as it's version-matched with the SDK.
133+
134+
Returns:
135+
Path to the binary if found, None otherwise
136+
"""
137+
try:
138+
import copilot as _copilot_mod # type: ignore[import-untyped]
139+
140+
mod_file = _copilot_mod.__file__
141+
if mod_file is None:
142+
logger.debug("[PLATFORM] copilot module has no __file__")
143+
return None
144+
145+
platform = get_platform_info()
146+
bin_dir = Path(mod_file).parent / CLI_BINARY_SUBDIR
147+
cli_bin = bin_dir / platform.cli_binary_name
148+
149+
if cli_bin.exists():
150+
logger.debug(f"[PLATFORM] Found SDK binary at: {cli_bin}")
151+
return cli_bin
152+
else:
153+
logger.debug(f"[PLATFORM] SDK binary not found at: {cli_bin}")
154+
return None
155+
156+
except ImportError:
157+
logger.debug("[PLATFORM] copilot SDK not installed")
158+
return None
159+
except Exception as e:
160+
logger.debug(f"[PLATFORM] Error locating SDK binary: {e}")
161+
return None
162+
163+
164+
def find_cli_in_path() -> Path | None:
165+
"""Find the CLI binary in system PATH.
166+
167+
Searches for both "copilot" and "copilot.exe" to handle cases where
168+
a Windows binary might be found on a Unix system (e.g., Wine) or
169+
vice versa.
170+
171+
Security Note (PATH Hijack Risk):
172+
This function uses shutil.which() which searches PATH directories
173+
in order. A malicious binary placed earlier in PATH could be
174+
executed instead of the legitimate CLI.
175+
176+
Mitigation:
177+
- This is a FALLBACK only — SDK bundled binary is preferred
178+
- locate_cli_binary() checks get_sdk_binary_path() first
179+
- PATH lookup only occurs when SDK binary is unavailable
180+
- Standard security practice: don't add untrusted dirs to PATH
181+
182+
Returns:
183+
Path to the binary if found, None otherwise
184+
"""
185+
# Try platform-appropriate name first
186+
platform = get_platform_info()
187+
found = shutil.which(platform.cli_binary_name)
188+
189+
if found:
190+
logger.debug(f"[PLATFORM] Found CLI in PATH: {found}")
191+
return Path(found)
192+
193+
# Fallback: try the other variant
194+
# This handles edge cases like Windows Subsystem for Linux with Windows PATH
195+
alternate_name = (
196+
CLI_BINARY_NAME_UNIX if platform.uses_exe_extension else CLI_BINARY_NAME_WINDOWS
197+
)
198+
found = shutil.which(alternate_name)
199+
200+
if found:
201+
logger.debug(f"[PLATFORM] Found CLI in PATH (alternate): {found}")
202+
return Path(found)
203+
204+
logger.debug("[PLATFORM] CLI not found in PATH")
205+
return None
206+
207+
208+
def locate_cli_binary() -> Path | None:
209+
"""Locate the CLI binary using the standard resolution order.
210+
211+
Resolution order (security-conscious):
212+
1. SDK bundled binary (PREFERRED, version-matched, tamper-resistant)
213+
2. System PATH fallback (only when SDK binary unavailable)
214+
215+
This is the main entry point for CLI location. It combines all
216+
discovery strategies and returns the first successful result.
217+
218+
Security Design:
219+
The SDK binary is ALWAYS preferred over PATH because:
220+
- It's bundled with the pip package (integrity verified)
221+
- It's version-matched with the SDK API
222+
- It's not susceptible to PATH hijacking attacks
223+
224+
PATH is only used when:
225+
- SDK is not installed via pip (e.g., development setup)
226+
- SDK binary path resolution fails (import error)
227+
228+
Returns:
229+
Path to the CLI binary, or None if not found
230+
"""
231+
# Strategy 1: SDK bundled binary (preferred - secure)
232+
sdk_path = get_sdk_binary_path()
233+
if sdk_path:
234+
return sdk_path
235+
236+
# Strategy 2: System PATH (fallback - less secure, see docstring)
237+
path_binary = find_cli_in_path()
238+
if path_binary:
239+
return path_binary
240+
241+
logger.debug("[PLATFORM] CLI binary not found via any strategy")
242+
return None
243+
244+
245+
# ═══════════════════════════════════════════════════════════════════════════════
246+
# Testing Utilities
247+
# ═══════════════════════════════════════════════════════════════════════════════
248+
249+
250+
def _make_test_platform_info(
251+
*,
252+
is_windows: bool = False,
253+
name: str | None = None,
254+
) -> PlatformInfo:
255+
"""Create a PlatformInfo for testing purposes.
256+
257+
This function is prefixed with underscore to indicate it's for testing.
258+
Use this instead of mocking sys.platform directly.
259+
260+
Args:
261+
is_windows: Whether to simulate Windows
262+
name: Optional custom name
263+
264+
Returns:
265+
PlatformInfo configured for testing
266+
"""
267+
if is_windows:
268+
return PlatformInfo(
269+
name=name or "Windows (test)",
270+
is_windows=True,
271+
cli_binary_name=CLI_BINARY_NAME_WINDOWS,
272+
uses_exe_extension=True,
273+
)
274+
else:
275+
return PlatformInfo(
276+
name=name or "Unix (test)",
277+
is_windows=False,
278+
cli_binary_name=CLI_BINARY_NAME_UNIX,
279+
uses_exe_extension=False,
280+
)

0 commit comments

Comments
 (0)