diff --git a/python/e2e/testharness/context.py b/python/e2e/testharness/context.py index 679a36e28..2171e25f2 100644 --- a/python/e2e/testharness/context.py +++ b/python/e2e/testharness/context.py @@ -11,18 +11,66 @@ import shutil import tempfile import time +from collections.abc import Sequence from pathlib import Path from typing import Any from copilot import CopilotClient, RuntimeConnection +from copilot._cli_version import get_npm_platform from .proxy import CapiProxy +def _cli_platform_package_names(npm_platform: str | None = None) -> list[str]: + """Return candidate ``@github/copilot-*`` directory names, best match first. + + Mirrors ``getCliPlatformPackageNames()`` in ``nodejs/src/client.ts``: as of CLI + 1.0.64-1 the runnable ``index.js`` ships in a platform package such as + ``copilot-darwin-arm64``. On Linux both libc variants are listed (the detected + one first) because npm installs exactly one of them and musl probing can come up + empty in minimal containers. + """ + primary = npm_platform or get_npm_platform() + names = [f"copilot-{primary}"] + if primary.startswith("linux"): + arch = primary.rsplit("-", 1)[-1] + for variant in (f"linux-{arch}", f"linuxmusl-{arch}"): + name = f"copilot-{variant}" + if name not in names: + names.append(name) + return names + + +def _find_cli_in_node_modules(github_modules: Path, package_names: Sequence[str]) -> str | None: + """Return the resolved ``index.js`` of the first installed candidate package. + + Only exact package names are probed, so unrelated ``copilot-*`` directories + (e.g. ``copilot-language-server``) can never be mistaken for the CLI. + """ + for name in package_names: + candidate = github_modules / name / "index.js" + if candidate.exists(): + return str(candidate.resolve()) + return None + + +def _installed_cli_package_names(github_modules: Path) -> list[str]: + """Return the ``copilot-*`` directory names present, for error messages only. + + Selection never globs — that was the #2103 bug. This exists so a failure can + say what *is* installed, which is the difference between a dead-end "run npm + install" and a message that diagnoses itself on a mixed-architecture host. + """ + if not github_modules.is_dir(): + return [] + return sorted(path.name for path in github_modules.glob("copilot-*") if path.is_dir()) + + def get_cli_path_for_tests() -> str: """Get CLI path for E2E tests. - Uses COPILOT_CLI_PATH env var if set, otherwise node_modules CLI. + Uses COPILOT_CLI_PATH env var if set, otherwise the platform-specific CLI + package in the sibling nodejs directory's node_modules. """ env_path = os.environ.get("COPILOT_CLI_PATH") if env_path and Path(env_path).exists(): @@ -30,15 +78,22 @@ def get_cli_path_for_tests() -> str: # Look for CLI in sibling nodejs directory's node_modules. As of CLI 1.0.64-1 # the @github/copilot package is a thin loader; the runnable index.js ships in - # the installed platform package (e.g. @github/copilot-linux-x64). + # the installed platform package (e.g. @github/copilot-linux-x64), so pick the + # one built for this host rather than whichever sorts first (#2103). base_path = Path(__file__).parents[3] github_modules = base_path / "nodejs" / "node_modules" / "@github" - for platform_pkg in sorted(github_modules.glob("copilot-*")): - candidate = platform_pkg / "index.js" - if candidate.exists(): - return str(candidate.resolve()) - - raise RuntimeError("CLI not found for tests. Run 'npm install' in the nodejs directory.") + package_names = _cli_platform_package_names() + found = _find_cli_in_node_modules(github_modules, package_names) + if found is not None: + return found + + installed = _installed_cli_package_names(github_modules) + raise RuntimeError( + f"CLI not found for tests under {github_modules} " + f"(tried: {', '.join(package_names)}; " + f"present: {', '.join(installed) or 'none'}). " + "Run 'npm install' in the nodejs directory, or set COPILOT_CLI_PATH." + ) CLI_PATH = get_cli_path_for_tests() diff --git a/python/test_e2e_harness_cli_path.py b/python/test_e2e_harness_cli_path.py new file mode 100644 index 000000000..8a50ba7a5 --- /dev/null +++ b/python/test_e2e_harness_cli_path.py @@ -0,0 +1,146 @@ +"""Unit tests for the E2E harness's Copilot CLI platform-package resolution. + +Regression coverage for github/copilot-sdk#2103: the harness used to return the +first ``@github/copilot-*`` directory in alphabetical order instead of the package +built for the current platform. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from copilot._cli_version import get_npm_platform +from e2e.testharness import context + + +def _make_package(github_modules: Path, name: str) -> Path: + """Create ``//index.js`` and return the entrypoint path.""" + package_dir = github_modules / name + package_dir.mkdir(parents=True, exist_ok=True) + index = package_dir / "index.js" + index.write_text("// fake CLI entrypoint\n") + return index + + +class TestCliPlatformPackageNames: + def test_non_linux_platform_yields_single_candidate(self): + assert context._cli_platform_package_names("darwin-arm64") == ["copilot-darwin-arm64"] + + def test_windows_platform_yields_single_candidate(self): + assert context._cli_platform_package_names("win32-x64") == ["copilot-win32-x64"] + + def test_glibc_linux_also_considers_musl_variant(self): + assert context._cli_platform_package_names("linux-x64") == [ + "copilot-linux-x64", + "copilot-linuxmusl-x64", + ] + + def test_musl_linux_prefers_musl_then_falls_back_to_glibc(self): + assert context._cli_platform_package_names("linuxmusl-arm64") == [ + "copilot-linuxmusl-arm64", + "copilot-linux-arm64", + ] + + def test_defaults_to_current_host_platform(self): + assert context._cli_platform_package_names()[0] == f"copilot-{get_npm_platform()}" + + +class TestFindCliInNodeModules: + def test_skips_alphabetically_earlier_foreign_package(self, tmp_path): + # The #2103 regression: "aardvark" sorts before every real platform name. + _make_package(tmp_path, "copilot-aardvark-x64") + expected = _make_package(tmp_path, "copilot-darwin-arm64") + found = context._find_cli_in_node_modules(tmp_path, ["copilot-darwin-arm64"]) + assert found == str(expected.resolve()) + + def test_returns_none_when_no_candidate_is_installed(self, tmp_path): + _make_package(tmp_path, "copilot-win32-x64") + assert context._find_cli_in_node_modules(tmp_path, ["copilot-darwin-arm64"]) is None + + def test_ignores_non_platform_copilot_packages(self, tmp_path): + _make_package(tmp_path, "copilot-language-server") + assert context._find_cli_in_node_modules(tmp_path, ["copilot-linux-x64"]) is None + + def test_prefers_earlier_candidate_when_both_libc_variants_exist(self, tmp_path): + expected = _make_package(tmp_path, "copilot-linuxmusl-x64") + _make_package(tmp_path, "copilot-linux-x64") + found = context._find_cli_in_node_modules( + tmp_path, ["copilot-linuxmusl-x64", "copilot-linux-x64"] + ) + assert found == str(expected.resolve()) + + def test_returns_none_when_package_dir_has_no_index_js(self, tmp_path): + (tmp_path / "copilot-linux-x64").mkdir() + assert context._find_cli_in_node_modules(tmp_path, ["copilot-linux-x64"]) is None + + def test_returns_none_when_github_modules_is_absent(self, tmp_path): + missing = tmp_path / "missing" + assert context._find_cli_in_node_modules(missing, ["copilot-linux-x64"]) is None + + +class TestInstalledCliPackageNames: + def test_lists_platform_directories_sorted(self, tmp_path): + _make_package(tmp_path, "copilot-win32-x64") + _make_package(tmp_path, "copilot-darwin-arm64") + (tmp_path / "not-copilot").mkdir() + assert context._installed_cli_package_names(tmp_path) == [ + "copilot-darwin-arm64", + "copilot-win32-x64", + ] + + def test_returns_empty_when_directory_is_absent(self, tmp_path): + assert context._installed_cli_package_names(tmp_path / "missing") == [] + + +class TestGetCliPathForTests: + def test_env_var_takes_precedence(self, tmp_path, monkeypatch): + cli = tmp_path / "custom-cli.js" + cli.write_text("// custom entrypoint\n") + monkeypatch.setenv("COPILOT_CLI_PATH", str(cli)) + assert context.get_cli_path_for_tests() == str(cli.resolve()) + + def test_error_names_the_packages_tried_and_the_remedy(self, monkeypatch): + monkeypatch.delenv("COPILOT_CLI_PATH", raising=False) + monkeypatch.setattr( + context, "_cli_platform_package_names", lambda *_: ["copilot-linux-x64"] + ) + monkeypatch.setattr(context, "_find_cli_in_node_modules", lambda *_: None) + with pytest.raises(RuntimeError) as excinfo: + context.get_cli_path_for_tests() + message = str(excinfo.value) + assert "copilot-linux-x64" in message + assert "npm install" in message + assert "COPILOT_CLI_PATH" in message + + def test_error_names_the_searched_directory(self, monkeypatch): + monkeypatch.delenv("COPILOT_CLI_PATH", raising=False) + seen: list[Path] = [] + + def fake_find(github_modules, package_names): + seen.append(github_modules) + return None + + monkeypatch.setattr(context, "_cli_platform_package_names", lambda *_: ["copilot-nope-x64"]) + monkeypatch.setattr(context, "_find_cli_in_node_modules", fake_find) + with pytest.raises(RuntimeError) as excinfo: + context.get_cli_path_for_tests() + assert seen, "get_cli_path_for_tests must consult _find_cli_in_node_modules" + assert seen[0].name == "@github" + assert seen[0].parent.name == "node_modules" + assert seen[0].parent.parent.name == "nodejs" + assert str(seen[0]) in str(excinfo.value) + + def test_error_lists_the_packages_actually_installed(self, monkeypatch): + monkeypatch.delenv("COPILOT_CLI_PATH", raising=False) + monkeypatch.setattr(context, "_cli_platform_package_names", lambda *_: ["copilot-nope-x64"]) + monkeypatch.setattr(context, "_find_cli_in_node_modules", lambda *_: None) + monkeypatch.setattr( + context, "_installed_cli_package_names", lambda *_: ["copilot-darwin-arm64"] + ) + with pytest.raises(RuntimeError) as excinfo: + context.get_cli_path_for_tests() + message = str(excinfo.value) + assert "present: copilot-darwin-arm64" in message + assert "copilot-nope-x64" in message