Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions clink/agents/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import logging
import os
import shlex
import shutil
import tempfile
import time
from collections.abc import Sequence
Expand All @@ -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

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.

medium

Since we can use shutil.which() directly in BaseCLIAgent.run() to avoid redundant PATH augmentation, we only need to import augment_path from clink.path_utils.

Suggested change
from clink.path_utils import augment_path, resolve_executable
from clink.path_utils import augment_path


logger = logging.getLogger("clink.agent")

Expand Down Expand Up @@ -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"))

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.

medium

In _build_environment(), env["PATH"] is already augmented with augment_path(env.get("PATH")). Calling resolve_executable(executable_name, path=env.get("PATH")) here results in a redundant call to augment_path(), which performs duplicate filesystem I/O (scanning directories, reading files, etc.).

Since env["PATH"] is already fully augmented, we can directly use shutil.which() to resolve the executable path efficiently.

        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}'. "
Expand Down Expand Up @@ -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

# ------------------------------------------------------------------
Expand Down
78 changes: 78 additions & 0 deletions clink/path_utils.py
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))

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.

high

The current implementation sorts the Node.js version directories lexicographically using sorted(..., reverse=True). Lexicographical sorting of version strings does not align with semantic versioning (e.g., "v9.0.0" is lexicographically greater than "v22.1.0" because '9' > '2'). This can cause older Node.js versions to be incorrectly preferred over newer ones when no default alias is set.

We should use a custom sorting key that parses the version components as integers to ensure correct semantic version ordering.

Suggested change
for node_bin in sorted(versions_dir.glob("*/bin"), reverse=True):
candidates.append(str(node_bin))
import re
def version_key(p: Path) -> list[int]:
return [int(x) for x in re.findall(r"\d+", p.parent.name)]
for node_bin in sorted(versions_dir.glob("*/bin"), key=version_key, reverse=True):
candidates.append(str(node_bin))


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)
7 changes: 4 additions & 3 deletions tests/test_clink_claude_agent.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import asyncio
import json
import shutil
from pathlib import Path

import pytest

from clink.agents import base as clink_base

from clink.agents.base import CLIAgentError
from clink.agents.claude import ClaudeAgent
from clink.models import ResolvedCLIClient, ResolvedCLIRole
Expand Down Expand Up @@ -46,11 +47,11 @@ async def _run_agent_with_process(monkeypatch, agent, role, process, *, system_p
async def fake_create_subprocess_exec(*_args, **_kwargs):
return process

def fake_which(executable_name):
def fake_resolve_executable(executable_name, *, path=None):
return f"/usr/bin/{executable_name}"

monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_create_subprocess_exec)
monkeypatch.setattr(shutil, "which", fake_which)
monkeypatch.setattr(clink_base, "resolve_executable", fake_resolve_executable)

return await agent.run(
role=role,
Expand Down
7 changes: 4 additions & 3 deletions tests/test_clink_codex_agent.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import asyncio
import shutil
from pathlib import Path

import pytest

from clink.agents import base as clink_base

from clink.agents.base import CLIAgentError
from clink.agents.codex import CodexAgent
from clink.models import ResolvedCLIClient, ResolvedCLIRole
Expand Down Expand Up @@ -42,11 +43,11 @@ async def _run_agent_with_process(monkeypatch, agent, role, process):
async def fake_create_subprocess_exec(*_args, **_kwargs):
return process

def fake_which(executable_name):
def fake_resolve_executable(executable_name, *, path=None):
return f"/usr/bin/{executable_name}"

monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_create_subprocess_exec)
monkeypatch.setattr(shutil, "which", fake_which)
monkeypatch.setattr(clink_base, "resolve_executable", fake_resolve_executable)
return await agent.run(role=role, prompt="do something", files=[], images=[])


Expand Down
7 changes: 4 additions & 3 deletions tests/test_clink_gemini_agent.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import asyncio
import shutil
from pathlib import Path

import pytest

from clink.agents import base as clink_base

from clink.agents.base import CLIAgentError
from clink.agents.gemini import GeminiAgent
from clink.models import ResolvedCLIClient, ResolvedCLIRole
Expand Down Expand Up @@ -42,11 +43,11 @@ async def _run_agent_with_process(monkeypatch, agent, role, process):
async def fake_create_subprocess_exec(*_args, **_kwargs):
return process

def fake_which(executable_name):
def fake_resolve_executable(executable_name, *, path=None):
return f"/usr/bin/{executable_name}"

monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_create_subprocess_exec)
monkeypatch.setattr(shutil, "which", fake_which)
monkeypatch.setattr(clink_base, "resolve_executable", fake_resolve_executable)
return await agent.run(role=role, prompt="do something", files=[], images=[])


Expand Down
86 changes: 86 additions & 0 deletions tests/test_clink_path_utils.py
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)
Loading