{{ message }}
python: remove implicit PATH fallback for CLI executable resolution - #2528
Draft
SteveSandersonMS wants to merge 1 commit into
Draft
python: remove implicit PATH fallback for CLI executable resolution#2528SteveSandersonMS wants to merge 1 commit into
SteveSandersonMS wants to merge 1 commit into
Conversation
The runtime start path fell back to shutil.which(cli_path) whenever the resolved cli_path did not exist as a file. This let the SDK implicitly select an arbitrary same-named executable found on the host PATH instead of failing when the SDK-managed runtime or an explicitly configured path was not actually present, re-introducing the PATH-search behavior originally added by 05dd60e that #2524 asks to remove. Resolution order is unchanged (explicit path > COPILOT_CLI_PATH > SDK-managed downloaded runtime, see _resolve_runtime_entrypoint); only the undocumented PATH-search fallback at process start is removed. A missing resolved path now raises a RuntimeError up front instead of silently searching PATH. Adds a regression test asserting that a same-named executable placed on PATH is not used when the configured path does not exist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of #2524 (Consolidate runtime discovery, acquisition, and embedding). Removes the Python SDK's implicit fallback to
PATH-based executable resolution, satisfying the required outcome:Background
Commit
05dd60e(#793) added ashutil.which(cli_path)fallback inCopilotClient._start_cli_serverthat runs whenever the resolvedcli_pathdoes not exist as a file. This let the SDK silently pick up an arbitrary same-named executable from the hostPATHinstead of using the runtime the SDK actually resolved (explicit path >COPILOT_CLI_PATH> SDK-managed download, see_resolve_runtime_entrypoint). #1934 flagged this explicitly for removal, and #2522/#2524 carry that forward.I confirmed on latest
main(0921029) and on the in-flightroji-unify-runtime-artifactsbranch (#2505) that this fallback is unchanged in both — #2505 only changes how the runtime is downloaded/verified from GitHub Releases for Python/.NET/Go/Java, not this PATH-search behavior at process start, so this is not duplicating that work.What changed
python/copilot/client.py:_start_cli_servernow raises a clearRuntimeErrorwhen the resolvedcli_pathdoesn't exist, instead of falling back toshutil.which. Removed the now-unusedshutilimport.python/test_client.py: added a regression test that plants a same-named decoy executable onPATHand asserts it is never used when the configured path is missing.Not in scope here (tracked separately on #2524)
bundled-in-processCargo feature) and Go (copilot_inprocessbuild tag) compile-time in-process embedding choices — see my findings comment on [v2] Consolidate runtime discovery, acquisition, and embedding #2524 for why I'm not changing these here and what maintainer input I'm requesting.Validation
ruff check/ruff format --checkon changed files: pass.ty checkoncopilot/client.py: pass.pytest python/test_client.py python/test_cli_download.py: same 79 pre-existing environment-only failures as unmodifiedmain(fake local CLI binary used for offline testing has no real e2e semantics); all path-resolution tests, including the new regression test, pass with zero new failures.Closes #2524 once merged (remaining #2524 items tracked in the follow-up comment on that issue).