fix(python): select the current platform's CLI package in the E2E harness - #2117
fix(python): select the current platform's CLI package in the E2E harness#2117nytron88 wants to merge 4 commits into
Conversation
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 github#2103
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 github#2103
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 github#2093, and name the tried packages in the error when none is installed. Fixes github#2103
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 github#2103
0f08409 to
818249f
Compare
|
@nytron88 Thanks, this is really helpful. Reusing copilot._cli_version.get_npm_platform() sounds like the right approach to me. I’d prefer to keep this fix scoped to the platform-aware package selection bug itself, so I would leave the lazy CLI_PATH resolution change out of this PR and track that separately if it still seems worth doing after this lands. The mixed-architecture fail-fast behavior also seems like the right trade-off here to me. If the current interpreter arch does not match the installed CLI package, surfacing that clearly is better than silently picking a different package, and COPILOT_CLI_PATH seems like a reasonable escape hatch for unusual setups. Also appreciate you filing the Go version separately. |
|
Thanks @syedkazmi14 — done, and agreed on both counts. Dropped the lazy The last one stays because it is part of the selection fix rather than the hardening: when One consequence worth stating plainly, since it is the reason I had bundled the change: with On the mixed-architecture trade-off — that matches my read too, so I have reworded that part Current state on this branch: 17 tests in |
Fixes #2103.
The Python E2E harness resolved the Copilot CLI by taking the first
@github/copilot-*directory in alphabetical order, so a tree with more than oneplatform package could launch an entrypoint built for another OS/arch — whichever
name sorted first won, regardless of the host. The glob could also match
non-platform packages such as
copilot-language-server.It now probes the exact platform package names for the host — reusing
copilot._cli_version.get_npm_platform(), which already owns the platform, arch andmusl-vs-glibc mapping — mirroring
getCliPlatformPackageNames()innodejs/src/client.tsand the .NET fix in #2093. On Linux both libc variants aretried, detected one first. When nothing matches, the error names the directory
searched, the packages tried, the packages actually present, and
COPILOT_CLI_PATH.New unit tests in
python/test_e2e_harness_cli_path.pycover this against fixturedirectory trees under
tmp_path, so no real CLI is ever launched.Behaviour change on mixed-architecture hosts
platform.machine()reflects the Python interpreter, but the resolvedindex.jsisexecuted by node (
python/copilot/client.py:3900-3901prependsnodefor.jspaths), and which platform package exists on disk was decided by whichever npm ran the
install. On a host where those differ — x86_64 CPython under Rosetta alongside native
arm64 node — the old loose glob happened to find
copilot-darwin-arm64and work, andthis change fails fast instead.
That is deliberate: silently launching a package built for a different architecture is
the bug being fixed.
COPILOT_CLI_PATHkeeps top precedence as the escape hatch and isnamed in the error, which also lists the packages actually present so the mismatch is
visible rather than mysterious.
Worth naming the divergence from #2093: it keys on the OS prefix only and globs
-*forthe arch, so it does not pin the architecture — this is the stricter of the two. Both
fail fast on the ordering bug itself (.NET errors on an ambiguous multi-package tree), so
the difference is narrowly about an arch mismatch on a single-package tree.
Note:
go/internal/e2e/testharness/context.go:36-41has the identical bug — filedseparately as #2116 to keep this PR scoped to the issue.