-
-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(clink): resolve npm-managed CLIs when nvm PATH is missing #455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |
| import logging | ||
| import os | ||
| import shlex | ||
| import shutil | ||
| import tempfile | ||
| import time | ||
| from collections.abc import Sequence | ||
|
|
@@ -16,6 +15,7 @@ | |
| from clink.constants import DEFAULT_STREAM_LIMIT | ||
| from clink.models import ResolvedCLIClient, ResolvedCLIRole | ||
| from clink.parsers import BaseParser, ParsedCLIResponse, ParserError, get_parser | ||
| from clink.path_utils import augment_path, resolve_executable | ||
|
|
||
| logger = logging.getLogger("clink.agent") | ||
|
|
||
|
|
@@ -70,7 +70,7 @@ async def run( | |
|
|
||
| # Resolve executable path for cross-platform compatibility (especially Windows) | ||
| executable_name = command[0] | ||
| resolved_executable = shutil.which(executable_name) | ||
| resolved_executable = resolve_executable(executable_name, path=env.get("PATH")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Since import shutil
resolved_executable = shutil.which(executable_name, path=env.get("PATH")) |
||
| if resolved_executable is None: | ||
| raise CLIAgentError( | ||
| f"Executable '{executable_name}' not found in PATH for CLI '{self.client.name}'. " | ||
|
|
@@ -201,6 +201,7 @@ def _build_command(self, *, role: ResolvedCLIRole, system_prompt: str | None) -> | |
| def _build_environment(self) -> dict[str, str]: | ||
| env = os.environ.copy() | ||
| env.update(self.client.env) | ||
| env["PATH"] = augment_path(env.get("PATH")) | ||
| return env | ||
|
|
||
| # ------------------------------------------------------------------ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,78 @@ | ||||||||||||||||
| """PATH augmentation helpers for resolving npm-managed CLI executables.""" | ||||||||||||||||
|
|
||||||||||||||||
| from __future__ import annotations | ||||||||||||||||
|
|
||||||||||||||||
| import os | ||||||||||||||||
| import shutil | ||||||||||||||||
| from pathlib import Path | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def _dedupe_path_entries(entries: list[str]) -> list[str]: | ||||||||||||||||
| seen: set[str] = set() | ||||||||||||||||
| ordered: list[str] = [] | ||||||||||||||||
| for entry in entries: | ||||||||||||||||
| if entry and entry not in seen: | ||||||||||||||||
| seen.add(entry) | ||||||||||||||||
| ordered.append(entry) | ||||||||||||||||
| return ordered | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def _nvm_node_bin_dirs(nvm_dir: Path) -> list[str]: | ||||||||||||||||
| """Return nvm node bin directories, preferring the default alias when set.""" | ||||||||||||||||
| candidates: list[str] = [] | ||||||||||||||||
|
|
||||||||||||||||
| default_alias = nvm_dir / "alias" / "default" | ||||||||||||||||
| if default_alias.is_file(): | ||||||||||||||||
| version = default_alias.read_text(encoding="utf-8").strip() | ||||||||||||||||
| if version and not version.startswith("v"): | ||||||||||||||||
| version = f"v{version}" | ||||||||||||||||
| alias_bin = nvm_dir / "versions" / "node" / version / "bin" | ||||||||||||||||
| if alias_bin.is_dir(): | ||||||||||||||||
| candidates.append(str(alias_bin)) | ||||||||||||||||
|
|
||||||||||||||||
| versions_dir = nvm_dir / "versions" / "node" | ||||||||||||||||
| if versions_dir.is_dir(): | ||||||||||||||||
| for node_bin in sorted(versions_dir.glob("*/bin"), reverse=True): | ||||||||||||||||
| candidates.append(str(node_bin)) | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation sorts the Node.js version directories lexicographically using We should use a custom sorting key that parses the version components as integers to ensure correct semantic version ordering.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| return candidates | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def collect_cli_path_candidates() -> list[str]: | ||||||||||||||||
| """Collect directories where npm-managed global CLIs are commonly installed.""" | ||||||||||||||||
| home = Path.home() | ||||||||||||||||
| candidates: list[str] = [] | ||||||||||||||||
|
|
||||||||||||||||
| nvm_dir = Path(os.environ.get("NVM_DIR", str(home / ".nvm"))) | ||||||||||||||||
| if nvm_dir.is_dir(): | ||||||||||||||||
| candidates.extend(_nvm_node_bin_dirs(nvm_dir)) | ||||||||||||||||
|
|
||||||||||||||||
| fnm_multishell = os.environ.get("FNM_MULTISHELL_PATH") | ||||||||||||||||
| if fnm_multishell: | ||||||||||||||||
| candidates.append(fnm_multishell) | ||||||||||||||||
|
|
||||||||||||||||
| for path in ( | ||||||||||||||||
| home / ".local" / "share" / "fnm" / "aliases" / "default" / "bin", | ||||||||||||||||
| home / ".volta" / "bin", | ||||||||||||||||
| home / ".asdf" / "shims", | ||||||||||||||||
| home / ".local" / "share" / "mise" / "shims", | ||||||||||||||||
| home / ".local" / "bin", | ||||||||||||||||
| home / ".npm-global" / "bin", | ||||||||||||||||
| ): | ||||||||||||||||
| if path.is_dir(): | ||||||||||||||||
| candidates.append(str(path)) | ||||||||||||||||
|
|
||||||||||||||||
| return _dedupe_path_entries(candidates) | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def augment_path(path: str | None = None) -> str: | ||||||||||||||||
| """Prepend common npm/node version-manager bin directories to PATH.""" | ||||||||||||||||
| current = path if path is not None else os.environ.get("PATH", "") | ||||||||||||||||
| current_parts = current.split(os.pathsep) if current else [] | ||||||||||||||||
| return os.pathsep.join(_dedupe_path_entries(collect_cli_path_candidates() + current_parts)) | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def resolve_executable(executable_name: str, *, path: str | None = None) -> str | None: | ||||||||||||||||
| """Resolve a CLI executable, searching augmented PATH when needed.""" | ||||||||||||||||
| search_path = augment_path(path) | ||||||||||||||||
| return shutil.which(executable_name, path=search_path) | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| """Tests for CLI executable PATH augmentation (issue #442).""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import stat | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
| from clink.path_utils import augment_path, collect_cli_path_candidates, resolve_executable | ||
|
|
||
|
|
||
| def test_collect_cli_path_candidates_includes_nvm_default_alias(tmp_path, monkeypatch): | ||
| nvm_dir = tmp_path / ".nvm" | ||
| bin_dir = nvm_dir / "versions" / "node" / "v22.1.0" / "bin" | ||
| bin_dir.mkdir(parents=True) | ||
| (nvm_dir / "alias" / "default").parent.mkdir(parents=True) | ||
| (nvm_dir / "alias" / "default").write_text("v22.1.0", encoding="utf-8") | ||
|
|
||
| monkeypatch.setenv("NVM_DIR", str(nvm_dir)) | ||
| monkeypatch.setenv("HOME", str(tmp_path)) | ||
|
|
||
| candidates = collect_cli_path_candidates() | ||
| assert str(bin_dir) in candidates | ||
|
|
||
|
|
||
| def test_collect_cli_path_candidates_includes_nvm_versions_without_alias(tmp_path, monkeypatch): | ||
| nvm_dir = tmp_path / ".nvm" | ||
| bin_dir = nvm_dir / "versions" / "node" / "v20.11.0" / "bin" | ||
| bin_dir.mkdir(parents=True) | ||
|
|
||
| monkeypatch.setenv("NVM_DIR", str(nvm_dir)) | ||
| monkeypatch.setenv("HOME", str(tmp_path)) | ||
|
|
||
| candidates = collect_cli_path_candidates() | ||
| assert str(bin_dir) in candidates | ||
|
|
||
|
|
||
| def test_augment_path_prepends_candidates_before_existing_entries(tmp_path, monkeypatch): | ||
| nvm_dir = tmp_path / ".nvm" | ||
| bin_dir = nvm_dir / "versions" / "node" / "v22.1.0" / "bin" | ||
| bin_dir.mkdir(parents=True) | ||
| (nvm_dir / "alias" / "default").parent.mkdir(parents=True) | ||
| (nvm_dir / "alias" / "default").write_text("v22.1.0", encoding="utf-8") | ||
|
|
||
| monkeypatch.setenv("NVM_DIR", str(nvm_dir)) | ||
| monkeypatch.setenv("HOME", str(tmp_path)) | ||
|
|
||
| augmented = augment_path("/usr/bin") | ||
| parts = augmented.split(os.pathsep) | ||
| assert parts[0] == str(bin_dir) | ||
| assert parts[-1] == "/usr/bin" | ||
|
|
||
|
|
||
| def test_resolve_executable_finds_binary_in_nvm_bin_dir(tmp_path, monkeypatch): | ||
| nvm_dir = tmp_path / ".nvm" | ||
| bin_dir = nvm_dir / "versions" / "node" / "v22.1.0" / "bin" | ||
| bin_dir.mkdir(parents=True) | ||
| codex_path = bin_dir / "codex" | ||
| codex_path.write_text("#!/bin/sh\necho codex\n", encoding="utf-8") | ||
| codex_path.chmod(codex_path.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) | ||
|
|
||
| (nvm_dir / "alias" / "default").parent.mkdir(parents=True) | ||
| (nvm_dir / "alias" / "default").write_text("v22.1.0", encoding="utf-8") | ||
|
|
||
| monkeypatch.setenv("NVM_DIR", str(nvm_dir)) | ||
| monkeypatch.setenv("HOME", str(tmp_path)) | ||
| monkeypatch.setenv("PATH", "/usr/bin") | ||
|
|
||
| resolved = resolve_executable("codex") | ||
| assert resolved == str(codex_path) | ||
|
|
||
|
|
||
| def test_resolve_executable_prefers_existing_path_entry(tmp_path, monkeypatch): | ||
| primary_bin = tmp_path / "primary" / "bin" | ||
| primary_bin.mkdir(parents=True) | ||
| codex_path = primary_bin / "codex" | ||
| codex_path.write_text("#!/bin/sh\necho primary\n", encoding="utf-8") | ||
| codex_path.chmod(codex_path.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) | ||
|
|
||
| monkeypatch.setenv("HOME", str(tmp_path)) | ||
| monkeypatch.setenv("PATH", str(primary_bin)) | ||
|
|
||
| resolved = resolve_executable("codex") | ||
| assert resolved == str(codex_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we can use
shutil.which()directly inBaseCLIAgent.run()to avoid redundant PATH augmentation, we only need to importaugment_pathfromclink.path_utils.