diff --git a/agent/lsp/cli.py b/agent/lsp/cli.py index 97a52c7c76c..c17ef682b33 100644 --- a/agent/lsp/cli.py +++ b/agent/lsp/cli.py @@ -140,6 +140,17 @@ def _cmd_status(emit_json: bool) -> int: disabled = info.get("disabled_servers") or [] if disabled: out.append(f" disabled in cfg: {', '.join(disabled)}") + + # Surface backend-tool gaps that aren't visible in the registry table: + # some servers spawn fine but emit no diagnostics without a sidecar + # binary (bash-language-server -> shellcheck). + backend_warnings = _backend_warnings() + if backend_warnings: + out.append("") + out.append("Backend warnings") + out.append("================") + for line in backend_warnings: + out.append(f" ! {line}") out.append("") out.append("Registered Servers") out.append("==================") @@ -268,3 +279,30 @@ def _recipe_pkg_for(server_id: str) -> str: "typescript": "typescript-language-server", } return aliases.get(server_id, server_id) + + +def _backend_warnings() -> list: + """Return human-readable notes about LSP backend tools that are missing + in a way that won't surface elsewhere. + + Some language servers ship as thin wrappers around an external CLI for + actual diagnostics — they spawn cleanly but never emit any errors when + the sidecar binary isn't on PATH. bash-language-server / shellcheck + is the load-bearing example. + + Returned strings are short, actionable, and include the install + suggestion across common platforms. + """ + import shutil as _shutil + from agent.lsp.install import hermes_lsp_bin_dir + notes: list = [] + bash_installed = _shutil.which("bash-language-server") is not None or ( + (hermes_lsp_bin_dir() / "bash-language-server").exists() + ) + if bash_installed and _shutil.which("shellcheck") is None: + notes.append( + "bash-language-server is installed but shellcheck is missing — " + "diagnostics will be empty (apt: shellcheck, brew: shellcheck, " + "scoop: shellcheck)." + ) + return notes diff --git a/agent/lsp/install.py b/agent/lsp/install.py index 5b5717dc014..0aaa22be744 100644 --- a/agent/lsp/install.py +++ b/agent/lsp/install.py @@ -33,7 +33,7 @@ import subprocess import sys import threading from pathlib import Path -from typing import Dict, Optional +from typing import Any, Dict, Optional logger = logging.getLogger("agent.lsp.install") @@ -41,7 +41,13 @@ logger = logging.getLogger("agent.lsp.install") # tuple of strategy name + package name + executable name. When the # install completes, we look for the executable in # ``/lsp/bin/`` first, then on PATH. -INSTALL_RECIPES: Dict[str, Dict[str, str]] = { +# +# Optional fields: +# - ``extra_pkgs``: list of sibling packages to install alongside +# ``pkg`` in the same node_modules tree. Used when an LSP server +# has a runtime peer dependency that npm doesn't auto-pull (e.g. +# typescript-language-server needs ``typescript``). +INSTALL_RECIPES: Dict[str, Dict[str, Any]] = { # Python "pyright": {"strategy": "npm", "pkg": "pyright", "bin": "pyright-langserver"}, # JS/TS family @@ -49,6 +55,11 @@ INSTALL_RECIPES: Dict[str, Dict[str, str]] = { "strategy": "npm", "pkg": "typescript-language-server", "bin": "typescript-language-server", + # typescript-language-server requires the `typescript` SDK + # (tsserver) to be importable from the same node_modules tree; + # otherwise initialize() fails with "Could not find a valid + # TypeScript installation". Install them together. + "extra_pkgs": ["typescript"], }, "@vue/language-server": { "strategy": "npm", @@ -179,7 +190,11 @@ def _do_install(pkg: str) -> Optional[str]: return None if strategy == "npm": - return _install_npm(recipe.get("pkg", pkg), bin_name) + return _install_npm( + recipe.get("pkg", pkg), + bin_name, + extra_pkgs=recipe.get("extra_pkgs") or [], + ) if strategy == "go": return _install_go(recipe.get("pkg", pkg), bin_name) if strategy == "pip": @@ -189,22 +204,36 @@ def _do_install(pkg: str) -> Optional[str]: return None -def _install_npm(pkg: str, bin_name: str) -> Optional[str]: +def _install_npm( + pkg: str, + bin_name: str, + extra_pkgs: Optional[list] = None, +) -> Optional[str]: """Install an npm package into our staging dir. Uses ``npm install --prefix`` so the binaries land in ``/node_modules/.bin/`` and we symlink them up one level for direct PATH-style access. + + ``extra_pkgs`` is a list of sibling packages to install in the + same ``node_modules`` tree. Used for LSP servers with runtime + peer deps that npm doesn't auto-pull (typescript-language-server + needs ``typescript`` next to it; intelephense ships standalone). """ npm = shutil.which("npm") if npm is None: logger.info("[install] cannot install %s: npm not on PATH", pkg) return None staging = hermes_lsp_bin_dir().parent # /lsp/ + install_targets = [pkg] + list(extra_pkgs or []) try: - logger.info("[install] npm install --prefix %s %s", staging, pkg) + logger.info( + "[install] npm install --prefix %s %s", + staging, + " ".join(install_targets), + ) proc = subprocess.run( - [npm, "install", "--prefix", str(staging), "--silent", "--no-fund", "--no-audit", pkg], + [npm, "install", "--prefix", str(staging), "--silent", "--no-fund", "--no-audit", *install_targets], check=False, capture_output=True, text=True, diff --git a/agent/lsp/servers.py b/agent/lsp/servers.py index df919fba991..00ad4c40005 100644 --- a/agent/lsp/servers.py +++ b/agent/lsp/servers.py @@ -336,6 +336,9 @@ def _spawn_clangd(root: str, ctx: ServerContext) -> Optional[SpawnSpec]: ) +_BASH_SHELLCHECK_WARNED = False + + def _spawn_bash_ls(root: str, ctx: ServerContext) -> Optional[SpawnSpec]: bin_path = _resolve_override(ctx, "bash-language-server") or _which("bash-language-server") if bin_path is None: @@ -343,6 +346,18 @@ def _spawn_bash_ls(root: str, ctx: ServerContext) -> Optional[SpawnSpec]: bin_path = try_install("bash-language-server", ctx.install_strategy) if bin_path is None: return None + # bash-language-server delegates diagnostics to ``shellcheck``. Without + # it on PATH the server starts and accepts requests but never reports + # any problems — to the user it looks like a working integration that + # never finds bugs. Warn once so the gap is visible. + global _BASH_SHELLCHECK_WARNED + if not _BASH_SHELLCHECK_WARNED and _which("shellcheck") is None: + _BASH_SHELLCHECK_WARNED = True + logger.warning( + "bash-language-server: shellcheck not found on PATH — " + "diagnostics will be empty until shellcheck is installed " + "(apt: shellcheck, brew: shellcheck, scoop: shellcheck)." + ) return SpawnSpec( command=[bin_path, "start"], workspace_root=root, diff --git a/tests/agent/lsp/test_install_and_lint_fixes.py b/tests/agent/lsp/test_install_and_lint_fixes.py new file mode 100644 index 00000000000..9046d01295e --- /dev/null +++ b/tests/agent/lsp/test_install_and_lint_fixes.py @@ -0,0 +1,279 @@ +"""Tests for follow-up fixes to the LSP integration (PR after #24168). + +Covers: + +1. ``typescript-language-server`` install recipe pulls in ``typescript`` + alongside the server, so the npm install command targets both. +2. ``hermes lsp status`` surfaces a ``Backend warnings`` section when + bash-language-server is installed but ``shellcheck`` is missing. +3. ``_check_lint`` returns ``skipped`` (not ``error``) when the linter + command exists on PATH but couldn't actually run — e.g. ``npx tsc`` + without the typescript SDK installed. This is what unblocks the + LSP semantic tier on TypeScript files when the user doesn't also + have a project-level ``tsc``. +""" +from __future__ import annotations + +import io +from contextlib import redirect_stdout +from unittest.mock import MagicMock, patch + +import pytest + +from agent.lsp.install import INSTALL_RECIPES + + +# --------------------------------------------------------------------------- +# Fix 1: typescript install recipe carries the typescript SDK +# --------------------------------------------------------------------------- + + +def test_typescript_recipe_includes_typescript_sdk(): + recipe = INSTALL_RECIPES["typescript-language-server"] + extras = recipe.get("extra_pkgs") or [] + assert "typescript" in extras, ( + "typescript-language-server requires the `typescript` SDK as a " + "sibling install — without it `initialize` fails with " + "'Could not find a valid TypeScript installation'." + ) + + +def test_install_npm_passes_extras_to_npm_command(tmp_path, monkeypatch): + """Verify the npm subprocess is invoked with both pkg AND extras.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + captured = {} + + def fake_run(cmd, **kwargs): + captured["cmd"] = cmd + # Pretend npm succeeded but binary doesn't exist — install code + # will return None, which is fine for this test. + return MagicMock(returncode=0, stderr="") + + from agent.lsp import install as install_mod + + monkeypatch.setattr(install_mod.subprocess, "run", fake_run) + monkeypatch.setattr(install_mod.shutil, "which", lambda c: "/usr/bin/npm" if c == "npm" else None) + + install_mod._install_npm("typescript-language-server", "typescript-language-server", + extra_pkgs=["typescript"]) + + cmd = captured["cmd"] + assert "typescript-language-server" in cmd + assert "typescript" in cmd + # Both must come AFTER the npm flags, in install-target position + install_idx = cmd.index("install") + assert cmd.index("typescript-language-server") > install_idx + assert cmd.index("typescript") > install_idx + + +def test_install_npm_works_without_extras(tmp_path, monkeypatch): + """Backwards compat: pyright-style recipes (no extras) still install.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + captured = {} + + def fake_run(cmd, **kwargs): + captured["cmd"] = cmd + return MagicMock(returncode=0, stderr="") + + from agent.lsp import install as install_mod + + monkeypatch.setattr(install_mod.subprocess, "run", fake_run) + monkeypatch.setattr(install_mod.shutil, "which", lambda c: "/usr/bin/npm" if c == "npm" else None) + + install_mod._install_npm("pyright", "pyright-langserver") + + cmd = captured["cmd"] + assert "pyright" in cmd + # Should not blow up when extra_pkgs is omitted/None + install_targets = [c for c in cmd if not c.startswith("-") and c not in ( + "install", "--prefix", str(install_mod.hermes_lsp_bin_dir().parent), + "/usr/bin/npm", + )] + assert install_targets == ["pyright"] + + +# --------------------------------------------------------------------------- +# Fix 2: ``hermes lsp status`` surfaces shellcheck-missing for bash +# --------------------------------------------------------------------------- + + +def test_backend_warnings_quiet_when_bash_not_installed(tmp_path, monkeypatch): + """No bash → no warning.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + from agent.lsp import cli as lsp_cli + + with patch("shutil.which", return_value=None): + notes = lsp_cli._backend_warnings() + assert notes == [] + + +def test_backend_warnings_quiet_when_bash_and_shellcheck_both_present(tmp_path, monkeypatch): + """Both installed → no warning.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + from agent.lsp import cli as lsp_cli + + def which(name): + return f"/usr/bin/{name}" # both found + + with patch("shutil.which", side_effect=which): + notes = lsp_cli._backend_warnings() + assert notes == [] + + +def test_backend_warnings_fires_when_bash_installed_but_shellcheck_missing(tmp_path, monkeypatch): + """The exact scenario from the bug report.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + from agent.lsp import cli as lsp_cli + + def which(name): + if name == "bash-language-server": + return "/fake/bin/bash-language-server" + return None # shellcheck missing + + with patch("shutil.which", side_effect=which): + notes = lsp_cli._backend_warnings() + assert len(notes) == 1 + assert "shellcheck" in notes[0].lower() + assert "bash-language-server" in notes[0].lower() + + +def test_status_output_includes_backend_warnings_section(tmp_path, monkeypatch): + """End-to-end: status command output includes the warning section.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + # Pretend bash-language-server is installed but shellcheck is missing + def which(name): + if name == "bash-language-server": + return "/fake/bin/bash-language-server" + return None + + from agent.lsp import cli as lsp_cli + + buf = io.StringIO() + with patch("shutil.which", side_effect=which), redirect_stdout(buf): + lsp_cli._cmd_status(emit_json=False) + + output = buf.getvalue() + assert "Backend warnings" in output + assert "shellcheck" in output + + +# --------------------------------------------------------------------------- +# Fix 3: tier-1 lint treats unusable linters as ``skipped``, not ``error`` +# --------------------------------------------------------------------------- + + +def test_npx_tsc_missing_treated_as_skipped(): + """The original bug: ``npx tsc`` errors when tsc isn't installed. + + Without this fix, the lint result is ``error``, which means the LSP + semantic tier (gated on ``success or skipped``) is skipped — the user + gets a useless tooling-error message instead of real diagnostics. + """ + from tools.file_operations import _looks_like_linter_unusable + + npx_failure_output = ( + " \n" + " This is not the tsc command you are looking for \n" + " \n" + "\n" + "To get access to the TypeScript compiler, tsc, from the command line either:\n" + "- Use npm install typescript to first add TypeScript to your project before using npx\n" + ) + + assert _looks_like_linter_unusable("npx", npx_failure_output) is True + + +def test_real_lint_error_not_classified_as_unusable(): + """A genuine TypeScript type error must NOT be misclassified.""" + from tools.file_operations import _looks_like_linter_unusable + + real_error = ( + "bad.ts:5:1 - error TS2322: Type 'number' is not assignable to type 'string'.\n" + "5 const x: string = greet(42);\n" + " ~~~~~~~~~~~~~~~\n" + ) + + assert _looks_like_linter_unusable("npx", real_error) is False + + +def test_unknown_base_cmd_returns_false(): + """Unfamiliar linters fall through and use the normal error path.""" + from tools.file_operations import _looks_like_linter_unusable + + assert _looks_like_linter_unusable("eslint", "any output") is False + assert _looks_like_linter_unusable("", "anything") is False + + +def test_check_lint_returns_skipped_when_npx_tsc_unusable(tmp_path): + """Integration: _check_lint sees npx exit non-zero with the npx banner + and returns a ``skipped`` LintResult so LSP can still run.""" + from tools.environments.local import LocalEnvironment + from tools.file_operations import ShellFileOperations + + ts_file = tmp_path / "bad.ts" + ts_file.write_text("const x: string = 42;\n") + + env = LocalEnvironment() + fops = ShellFileOperations(env) + + # Patch _exec to simulate ``npx tsc`` failing because tsc is missing. + npx_banner = ( + " \n" + " This is not the tsc command you are looking for \n" + ) + + def fake_exec(cmd, **kwargs): + result = MagicMock() + result.exit_code = 1 + result.stdout = npx_banner + return result + + with patch.object(fops, "_exec", side_effect=fake_exec), \ + patch.object(fops, "_has_command", return_value=True): + lint = fops._check_lint(str(ts_file)) + + assert lint.skipped is True, ( + f"expected skipped (so LSP runs); got success={lint.success}, " + f"output={lint.output!r}" + ) + assert "not usable" in (lint.message or "") + + +def test_check_lint_returns_error_for_real_ts_type_errors(tmp_path): + """Sanity: real TypeScript errors still go through the error path.""" + from tools.environments.local import LocalEnvironment + from tools.file_operations import ShellFileOperations + + ts_file = tmp_path / "bad.ts" + ts_file.write_text("const x: string = 42;\n") + + env = LocalEnvironment() + fops = ShellFileOperations(env) + + real_tsc_error = ( + "bad.ts:1:7 - error TS2322: Type 'number' is not assignable to type 'string'.\n" + "1 const x: string = 42;\n" + " ~\n" + "Found 1 error.\n" + ) + + def fake_exec(cmd, **kwargs): + result = MagicMock() + result.exit_code = 1 + result.stdout = real_tsc_error + return result + + with patch.object(fops, "_exec", side_effect=fake_exec), \ + patch.object(fops, "_has_command", return_value=True): + lint = fops._check_lint(str(ts_file)) + + assert lint.skipped is False + assert lint.success is False + assert "TS2322" in lint.output + + +if __name__ == "__main__": # pragma: no cover + pytest.main([__file__, "-v"]) diff --git a/tools/file_operations.py b/tools/file_operations.py index f8b194b215c..4b64421622f 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -327,6 +327,55 @@ LINTERS = { } +# Patterns that indicate the linter base command exists on PATH but +# couldn't actually run — e.g. ``npx tsc`` when tsc isn't installed in +# node_modules, or rustfmt complaining there's no Cargo project. When +# any of these substrings appears in the linter output, ``_check_lint`` +# returns ``skipped`` instead of ``error`` so: +# +# 1. The write isn't flagged for a tooling problem the agent can't fix. +# 2. The LSP semantic tier still runs (it gates on success/skipped). +# +# Patterns are matched case-insensitively against linter stdout. +_LINTER_UNUSABLE_PATTERNS = { + 'npx': ( + # npx prints this banner when the package isn't installed locally + # AND it can't auto-install (no internet, registry off, etc.) or + # when the binary it tried to run is the wrong one. + 'this is not the tsc command you are looking for', + # npx with --no-install resolution failures + 'could not determine executable to run', + 'not found in npm registry', + ), + 'rustfmt': ( + # rustfmt outside a Cargo project + 'no input filename given', + 'error: not a workspace', + ), + 'go': ( + # ``go vet`` on a file outside a module / GOPATH + 'cannot find package', + 'go: cannot find main module', + ), +} + + +def _looks_like_linter_unusable(base_cmd: str, output: str) -> bool: + """Return True iff ``output`` from ``base_cmd`` indicates the linter + itself couldn't run (a tooling gap), as opposed to a real lint error + in the file being checked. + + ``base_cmd`` is the first word of the linter command line (``npx``, + ``rustfmt``, ``go``, ...). ``output`` is the stdout/stderr captured + from running it. + """ + patterns = _LINTER_UNUSABLE_PATTERNS.get(base_cmd) + if not patterns: + return False + lower = output.lower() + return any(p in lower for p in patterns) + + def _lint_json_inproc(content: str) -> tuple[bool, str]: """In-process JSON syntax check. Returns (ok, error_message).""" import json as _json @@ -1117,6 +1166,24 @@ class ShellFileOperations(FileOperations): cmd = linter_cmd.replace("{file}", self._escape_shell_arg(path)) result = self._exec(cmd, timeout=30) + if result.exit_code != 0 and _looks_like_linter_unusable(base_cmd, result.stdout): + # The linter command exists on PATH but couldn't actually run + # (e.g. ``npx tsc`` when tsc isn't in node_modules; ``rustfmt + # --check`` without a Cargo project). This is a tooling gap, + # not a real lint failure — surface it as ``skipped`` so the + # write doesn't get flagged AND so the LSP tier still runs. + from tools.ansi_strip import strip_ansi + cleaned = strip_ansi(result.stdout).strip() + # Collapse to a single line — the npx banner is multi-line ASCII. + first_line = next( + (ln.strip() for ln in cleaned.splitlines() if ln.strip()), + cleaned[:120], + ) + return LintResult( + skipped=True, + message=f"{base_cmd} not usable: {first_line[:200]}", + ) + return LintResult( success=result.exit_code == 0, output=result.stdout.strip() if result.stdout.strip() else ""