Skip to content

Commit b733d53

Browse files
authored
fix(python): select the current platform's CLI package in the E2E harness (#2117)
* test(python): resolve E2E CLI candidates by host platform Adds _cli_platform_package_names(), mirroring getCliPlatformPackageNames() in nodejs/src/client.ts, reusing copilot._cli_version.get_npm_platform() so the platform/arch/musl mapping lives in exactly one place. Refs #2103 * test(python): probe exact CLI platform package names on disk Adds _find_cli_in_node_modules(), which looks up only the exact candidate package names for the host. Unrelated @github/copilot-* directories such as copilot-language-server can no longer be mistaken for the CLI entrypoint. Wired into get_cli_path_for_tests() in the following commit. Refs #2103 * fix(python): select the current platform's CLI package in E2E harness The harness returned the first @github/copilot-* directory in alphabetical order, so a tree containing more than one platform package (stale node_modules, cross-platform mount, restored cache) launched an entrypoint built for another OS/arch. It also matched non-platform packages such as copilot-language-server. Probe the exact platform package names for the host instead, mirroring nodejs/src/client.ts and the .NET fix in #2093, and name the tried packages in the error when none is installed. Fixes #2103 * fix(python): name the installed CLI packages when resolution fails The failure told the reader to run 'npm install' without saying what was already there. On a host whose Python and node architectures differ, the package npm installed is present and correct for node while the harness asks for the interpreter's architecture, so 'npm install' is a dead end and the message gave no way to see that. List the copilot-* directories actually present alongside the ones tried. Selection still probes exact names only; the glob is confined to the error path. Refs #2103
1 parent ad67361 commit b733d53

2 files changed

Lines changed: 209 additions & 8 deletions

File tree

python/e2e/testharness/context.py

Lines changed: 63 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,34 +11,89 @@
1111
import shutil
1212
import tempfile
1313
import time
14+
from collections.abc import Sequence
1415
from pathlib import Path
1516
from typing import Any
1617

1718
from copilot import CopilotClient, RuntimeConnection
19+
from copilot._cli_version import get_npm_platform
1820

1921
from .proxy import CapiProxy
2022

2123

24+
def _cli_platform_package_names(npm_platform: str | None = None) -> list[str]:
25+
"""Return candidate ``@github/copilot-*`` directory names, best match first.
26+
27+
Mirrors ``getCliPlatformPackageNames()`` in ``nodejs/src/client.ts``: as of CLI
28+
1.0.64-1 the runnable ``index.js`` ships in a platform package such as
29+
``copilot-darwin-arm64``. On Linux both libc variants are listed (the detected
30+
one first) because npm installs exactly one of them and musl probing can come up
31+
empty in minimal containers.
32+
"""
33+
primary = npm_platform or get_npm_platform()
34+
names = [f"copilot-{primary}"]
35+
if primary.startswith("linux"):
36+
arch = primary.rsplit("-", 1)[-1]
37+
for variant in (f"linux-{arch}", f"linuxmusl-{arch}"):
38+
name = f"copilot-{variant}"
39+
if name not in names:
40+
names.append(name)
41+
return names
42+
43+
44+
def _find_cli_in_node_modules(github_modules: Path, package_names: Sequence[str]) -> str | None:
45+
"""Return the resolved ``index.js`` of the first installed candidate package.
46+
47+
Only exact package names are probed, so unrelated ``copilot-*`` directories
48+
(e.g. ``copilot-language-server``) can never be mistaken for the CLI.
49+
"""
50+
for name in package_names:
51+
candidate = github_modules / name / "index.js"
52+
if candidate.exists():
53+
return str(candidate.resolve())
54+
return None
55+
56+
57+
def _installed_cli_package_names(github_modules: Path) -> list[str]:
58+
"""Return the ``copilot-*`` directory names present, for error messages only.
59+
60+
Selection never globs — that was the #2103 bug. This exists so a failure can
61+
say what *is* installed, which is the difference between a dead-end "run npm
62+
install" and a message that diagnoses itself on a mixed-architecture host.
63+
"""
64+
if not github_modules.is_dir():
65+
return []
66+
return sorted(path.name for path in github_modules.glob("copilot-*") if path.is_dir())
67+
68+
2269
def get_cli_path_for_tests() -> str:
2370
"""Get CLI path for E2E tests.
2471
25-
Uses COPILOT_CLI_PATH env var if set, otherwise node_modules CLI.
72+
Uses COPILOT_CLI_PATH env var if set, otherwise the platform-specific CLI
73+
package in the sibling nodejs directory's node_modules.
2674
"""
2775
env_path = os.environ.get("COPILOT_CLI_PATH")
2876
if env_path and Path(env_path).exists():
2977
return str(Path(env_path).resolve())
3078

3179
# Look for CLI in sibling nodejs directory's node_modules. As of CLI 1.0.64-1
3280
# the @github/copilot package is a thin loader; the runnable index.js ships in
33-
# the installed platform package (e.g. @github/copilot-linux-x64).
81+
# the installed platform package (e.g. @github/copilot-linux-x64), so pick the
82+
# one built for this host rather than whichever sorts first (#2103).
3483
base_path = Path(__file__).parents[3]
3584
github_modules = base_path / "nodejs" / "node_modules" / "@github"
36-
for platform_pkg in sorted(github_modules.glob("copilot-*")):
37-
candidate = platform_pkg / "index.js"
38-
if candidate.exists():
39-
return str(candidate.resolve())
40-
41-
raise RuntimeError("CLI not found for tests. Run 'npm install' in the nodejs directory.")
85+
package_names = _cli_platform_package_names()
86+
found = _find_cli_in_node_modules(github_modules, package_names)
87+
if found is not None:
88+
return found
89+
90+
installed = _installed_cli_package_names(github_modules)
91+
raise RuntimeError(
92+
f"CLI not found for tests under {github_modules} "
93+
f"(tried: {', '.join(package_names)}; "
94+
f"present: {', '.join(installed) or 'none'}). "
95+
"Run 'npm install' in the nodejs directory, or set COPILOT_CLI_PATH."
96+
)
4297

4398

4499
CLI_PATH = get_cli_path_for_tests()
Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
"""Unit tests for the E2E harness's Copilot CLI platform-package resolution.
2+
3+
Regression coverage for github/copilot-sdk#2103: the harness used to return the
4+
first ``@github/copilot-*`` directory in alphabetical order instead of the package
5+
built for the current platform.
6+
"""
7+
8+
from __future__ import annotations
9+
10+
from pathlib import Path
11+
12+
import pytest
13+
14+
from copilot._cli_version import get_npm_platform
15+
from e2e.testharness import context
16+
17+
18+
def _make_package(github_modules: Path, name: str) -> Path:
19+
"""Create ``<github_modules>/<name>/index.js`` and return the entrypoint path."""
20+
package_dir = github_modules / name
21+
package_dir.mkdir(parents=True, exist_ok=True)
22+
index = package_dir / "index.js"
23+
index.write_text("// fake CLI entrypoint\n")
24+
return index
25+
26+
27+
class TestCliPlatformPackageNames:
28+
def test_non_linux_platform_yields_single_candidate(self):
29+
assert context._cli_platform_package_names("darwin-arm64") == ["copilot-darwin-arm64"]
30+
31+
def test_windows_platform_yields_single_candidate(self):
32+
assert context._cli_platform_package_names("win32-x64") == ["copilot-win32-x64"]
33+
34+
def test_glibc_linux_also_considers_musl_variant(self):
35+
assert context._cli_platform_package_names("linux-x64") == [
36+
"copilot-linux-x64",
37+
"copilot-linuxmusl-x64",
38+
]
39+
40+
def test_musl_linux_prefers_musl_then_falls_back_to_glibc(self):
41+
assert context._cli_platform_package_names("linuxmusl-arm64") == [
42+
"copilot-linuxmusl-arm64",
43+
"copilot-linux-arm64",
44+
]
45+
46+
def test_defaults_to_current_host_platform(self):
47+
assert context._cli_platform_package_names()[0] == f"copilot-{get_npm_platform()}"
48+
49+
50+
class TestFindCliInNodeModules:
51+
def test_skips_alphabetically_earlier_foreign_package(self, tmp_path):
52+
# The #2103 regression: "aardvark" sorts before every real platform name.
53+
_make_package(tmp_path, "copilot-aardvark-x64")
54+
expected = _make_package(tmp_path, "copilot-darwin-arm64")
55+
found = context._find_cli_in_node_modules(tmp_path, ["copilot-darwin-arm64"])
56+
assert found == str(expected.resolve())
57+
58+
def test_returns_none_when_no_candidate_is_installed(self, tmp_path):
59+
_make_package(tmp_path, "copilot-win32-x64")
60+
assert context._find_cli_in_node_modules(tmp_path, ["copilot-darwin-arm64"]) is None
61+
62+
def test_ignores_non_platform_copilot_packages(self, tmp_path):
63+
_make_package(tmp_path, "copilot-language-server")
64+
assert context._find_cli_in_node_modules(tmp_path, ["copilot-linux-x64"]) is None
65+
66+
def test_prefers_earlier_candidate_when_both_libc_variants_exist(self, tmp_path):
67+
expected = _make_package(tmp_path, "copilot-linuxmusl-x64")
68+
_make_package(tmp_path, "copilot-linux-x64")
69+
found = context._find_cli_in_node_modules(
70+
tmp_path, ["copilot-linuxmusl-x64", "copilot-linux-x64"]
71+
)
72+
assert found == str(expected.resolve())
73+
74+
def test_returns_none_when_package_dir_has_no_index_js(self, tmp_path):
75+
(tmp_path / "copilot-linux-x64").mkdir()
76+
assert context._find_cli_in_node_modules(tmp_path, ["copilot-linux-x64"]) is None
77+
78+
def test_returns_none_when_github_modules_is_absent(self, tmp_path):
79+
missing = tmp_path / "missing"
80+
assert context._find_cli_in_node_modules(missing, ["copilot-linux-x64"]) is None
81+
82+
83+
class TestInstalledCliPackageNames:
84+
def test_lists_platform_directories_sorted(self, tmp_path):
85+
_make_package(tmp_path, "copilot-win32-x64")
86+
_make_package(tmp_path, "copilot-darwin-arm64")
87+
(tmp_path / "not-copilot").mkdir()
88+
assert context._installed_cli_package_names(tmp_path) == [
89+
"copilot-darwin-arm64",
90+
"copilot-win32-x64",
91+
]
92+
93+
def test_returns_empty_when_directory_is_absent(self, tmp_path):
94+
assert context._installed_cli_package_names(tmp_path / "missing") == []
95+
96+
97+
class TestGetCliPathForTests:
98+
def test_env_var_takes_precedence(self, tmp_path, monkeypatch):
99+
cli = tmp_path / "custom-cli.js"
100+
cli.write_text("// custom entrypoint\n")
101+
monkeypatch.setenv("COPILOT_CLI_PATH", str(cli))
102+
assert context.get_cli_path_for_tests() == str(cli.resolve())
103+
104+
def test_error_names_the_packages_tried_and_the_remedy(self, monkeypatch):
105+
monkeypatch.delenv("COPILOT_CLI_PATH", raising=False)
106+
monkeypatch.setattr(
107+
context, "_cli_platform_package_names", lambda *_: ["copilot-linux-x64"]
108+
)
109+
monkeypatch.setattr(context, "_find_cli_in_node_modules", lambda *_: None)
110+
with pytest.raises(RuntimeError) as excinfo:
111+
context.get_cli_path_for_tests()
112+
message = str(excinfo.value)
113+
assert "copilot-linux-x64" in message
114+
assert "npm install" in message
115+
assert "COPILOT_CLI_PATH" in message
116+
117+
def test_error_names_the_searched_directory(self, monkeypatch):
118+
monkeypatch.delenv("COPILOT_CLI_PATH", raising=False)
119+
seen: list[Path] = []
120+
121+
def fake_find(github_modules, package_names):
122+
seen.append(github_modules)
123+
return None
124+
125+
monkeypatch.setattr(context, "_cli_platform_package_names", lambda *_: ["copilot-nope-x64"])
126+
monkeypatch.setattr(context, "_find_cli_in_node_modules", fake_find)
127+
with pytest.raises(RuntimeError) as excinfo:
128+
context.get_cli_path_for_tests()
129+
assert seen, "get_cli_path_for_tests must consult _find_cli_in_node_modules"
130+
assert seen[0].name == "@github"
131+
assert seen[0].parent.name == "node_modules"
132+
assert seen[0].parent.parent.name == "nodejs"
133+
assert str(seen[0]) in str(excinfo.value)
134+
135+
def test_error_lists_the_packages_actually_installed(self, monkeypatch):
136+
monkeypatch.delenv("COPILOT_CLI_PATH", raising=False)
137+
monkeypatch.setattr(context, "_cli_platform_package_names", lambda *_: ["copilot-nope-x64"])
138+
monkeypatch.setattr(context, "_find_cli_in_node_modules", lambda *_: None)
139+
monkeypatch.setattr(
140+
context, "_installed_cli_package_names", lambda *_: ["copilot-darwin-arm64"]
141+
)
142+
with pytest.raises(RuntimeError) as excinfo:
143+
context.get_cli_path_for_tests()
144+
message = str(excinfo.value)
145+
assert "present: copilot-darwin-arm64" in message
146+
assert "copilot-nope-x64" in message

0 commit comments

Comments
 (0)