Skip to content

fix(python): select the current platform's CLI package in the E2E harness - #2117

Merged
SteveSandersonMS merged 4 commits into
github:mainfrom
nytron88:fix/2103-python-e2e-cli-platform-resolution
Jul 30, 2026
Merged

fix(python): select the current platform's CLI package in the E2E harness#2117
SteveSandersonMS merged 4 commits into
github:mainfrom
nytron88:fix/2103-python-e2e-cli-platform-resolution

Conversation

@nytron88

@nytron88 nytron88 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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 one
platform 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 and
musl-vs-glibc mapping — mirroring getCliPlatformPackageNames() in
nodejs/src/client.ts and the .NET fix in #2093. On Linux both libc variants are
tried, 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.py cover this against fixture
directory 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 resolved index.js is
executed by node (python/copilot/client.py:3900-3901 prepends node for .js
paths), 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-arm64 and work, and
this change fails fast instead.

That is deliberate: silently launching a package built for a different architecture is
the bug being fixed. COPILOT_CLI_PATH keeps top precedence as the escape hatch and is
named 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 -* for
the 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-41 has the identical bug — filed
separately as #2116 to keep this PR scoped to the issue.

nytron88 added 3 commits July 28, 2026 18:09
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
Copilot AI review requested due to automatic review settings July 29, 2026 00:59
@nytron88
nytron88 requested a review from a team as a code owner July 29, 2026 00:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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
@nytron88
nytron88 requested a review from Copilot July 29, 2026 01:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings July 29, 2026 01:12
@nytron88
nytron88 force-pushed the fix/2103-python-e2e-cli-platform-resolution branch from 0f08409 to 818249f Compare July 29, 2026 01:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@syedkazmi14

Copy link
Copy Markdown

@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.

@nytron88

Copy link
Copy Markdown
Contributor Author

Thanks @syedkazmi14 — done, and agreed on both counts.

Dropped the lazy CLI_PATH change. Force-pushed; the branch is four commits now:

818249f2 fix(python): name the installed CLI packages when resolution fails
5a7105a0 fix(python): select the current platform's CLI package in E2E harness
469847bd test(python): probe exact CLI platform package names on disk
e164fb24 test(python): resolve E2E CLI candidates by host platform

The last one stays because it is part of the selection fix rather than the hardening: when
nothing matches, the error now lists the copilot-* packages actually present alongside the
ones tried. That matters specifically for the mixed-arch case below — otherwise the message
tells you to run npm install while the package npm would install is already sitting in that
directory, which is a dead end with no way to see why.

One consequence worth stating plainly, since it is the reason I had bundled the change: with
that commit out, CLI_PATH = get_cli_path_for_tests() runs at module scope again, so on a host
that cannot resolve a matching package the new unit tests fail at collection rather than as
test failures — even though they only use tmp_path fixtures and need no CLI. CI is unaffected
(npm ci --ignore-scripts runs in nodejs/ before pytest), so this only shows up on a fresh
clone or a stale worktree locally. Happy to leave it exactly as you suggested; I have parked the
work on spike/lazy-cli-path-2103
and will open an issue after this lands if it still looks worth doing.

On the mixed-architecture trade-off — that matches my read too, so I have reworded that part
of the PR description from an open question into a plain note about the behaviour change, and
kept the detail that the divergence from #2093 is intentional (that one keys on the OS prefix
only and does not pin the arch, so this is the stricter of the two).

Current state on this branch: 17 tests in python/test_e2e_harness_cli_path.py, 302 in the flat
unit suite, e2e/test_client_e2e.py 14/14 against the real CLI, ruff format --check and
ruff check clean on each commit individually.

@nytron88
nytron88 requested a review from Copilot July 29, 2026 03:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@nytron88
nytron88 requested a review from Copilot July 29, 2026 16:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@SteveSandersonMS
SteveSandersonMS merged commit b733d53 into github:main Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Python][tests] E2E harness picks the first @github/copilot-* package instead of the current platform package

4 participants