From ed85ae25b246932bfa57c3d59ab752d6c58b0c09 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:09:52 -0700 Subject: [PATCH] refactor(agent/lsp): dedupe status lookups in cli, compact docstrings and reader dispatch --- agent/lsp/cli.py | 93 ++++++++++++++------------------ agent/lsp/client.py | 122 ++++++++++++++++-------------------------- agent/lsp/eventlog.py | 17 +++--- agent/lsp/install.py | 18 +++---- agent/lsp/manager.py | 66 ++++++++++------------- agent/lsp/protocol.py | 9 ++-- agent/lsp/reporter.py | 7 ++- agent/lsp/servers.py | 12 ++--- 8 files changed, 137 insertions(+), 207 deletions(-) diff --git a/agent/lsp/cli.py b/agent/lsp/cli.py index 28484ff2a7..bc03021ed5 100644 --- a/agent/lsp/cli.py +++ b/agent/lsp/cli.py @@ -9,18 +9,23 @@ import sys _STATUS_MARKERS = {"installed": "✓", "missing": "·", "manual-only": "?"} -# (name, parser kwargs, optional (flag, add_argument kwargs)) — order defines the help listing. +# (name, help, optional (flag, add_argument kwargs), handler(args)) — order defines the help listing. _SUBCOMMANDS = [ - ("status", {"help": "Show LSP service status"}, - ("--json", {"action": "store_true", "help": "Emit machine-readable JSON"})), - ("list", {"help": "List supported language servers"}, - ("--installed-only", {"action": "store_true", "help": "Only show servers whose binary is currently available"})), - ("install", {"help": "Install a server binary"}, ("server", {"help": "Server id (e.g. pyright, gopls)"})), - ("install-all", {"help": "Install every server with a known auto-install recipe"}, - ("--include-manual", {"action": "store_true", "help": "Even attempt servers marked manual-install (best effort)"})), - ("restart", {"help": "Tear down running LSP clients (next edit re-spawns)"}, None), - ("which", {"help": "Print binary path for a server"}, ("server", {"help": "Server id"})), + ("status", "Show LSP service status", + ("--json", {"action": "store_true", "help": "Emit machine-readable JSON"}), + lambda a: _cmd_status(getattr(a, "json", False))), + ("list", "List supported language servers", + ("--installed-only", {"action": "store_true", "help": "Only show servers whose binary is currently available"}), + lambda a: _cmd_list(getattr(a, "installed_only", False))), + ("install", "Install a server binary", ("server", {"help": "Server id (e.g. pyright, gopls)"}), + lambda a: _cmd_install(a.server)), + ("install-all", "Install every server with a known auto-install recipe", + ("--include-manual", {"action": "store_true", "help": "Even attempt servers marked manual-install (best effort)"}), + lambda a: _cmd_install_all(getattr(a, "include_manual", False))), + ("restart", "Tear down running LSP clients (next edit re-spawns)", None, lambda a: _cmd_restart()), + ("which", "Print binary path for a server", ("server", {"help": "Server id"}), lambda a: _cmd_which(a.server)), ] +_COMMANDS = {name: handler for name, _, _, handler in _SUBCOMMANDS} def register_subparser(subparsers: argparse._SubParsersAction) -> None: @@ -31,23 +36,13 @@ def register_subparser(subparsers: argparse._SubParsersAction) -> None: description="Manage the LSP layer that powers post-write semantic diagnostics in write_file/patch.", ) sub = parser.add_subparsers(dest="lsp_command") - for name, kwargs, arg in _SUBCOMMANDS: - p = sub.add_parser(name, **kwargs) + for name, help_text, arg, _handler in _SUBCOMMANDS: + p = sub.add_parser(name, help=help_text) if arg is not None: p.add_argument(arg[0], **arg[1]) parser.set_defaults(func=run_lsp_command) -_COMMANDS = { - "status": lambda a: _cmd_status(getattr(a, "json", False)), - "list": lambda a: _cmd_list(getattr(a, "installed_only", False)), - "install": lambda a: _cmd_install(a.server), - "install-all": lambda a: _cmd_install_all(getattr(a, "include_manual", False)), - "restart": lambda a: _cmd_restart(), - "which": lambda a: _cmd_which(a.server), -} - - def run_lsp_command(args: argparse.Namespace) -> int: """Top-level dispatcher for ``hermes lsp ``.""" sub = getattr(args, "lsp_command", None) or "status" @@ -61,21 +56,20 @@ def run_lsp_command(args: argparse.Namespace) -> int: return 130 +def _status_for(server_id: str) -> str: + from agent.lsp.install import detect_status + return detect_status(_recipe_pkg_for(server_id)) + + def _cmd_status(emit_json: bool) -> int: from agent.lsp import get_service from agent.lsp.servers import SERVERS - from agent.lsp.install import detect_status - svc = get_service() info = svc.get_status() if svc is not None else {"enabled": False} - if emit_json: import json - registry = [ - {"server_id": s.server_id, "extensions": list(s.extensions), "description": s.description, - "binary_status": detect_status(_recipe_pkg_for(s.server_id))} - for s in SERVERS - ] + registry = [{"server_id": s.server_id, "extensions": list(s.extensions), "description": s.description, + "binary_status": _status_for(s.server_id)} for s in SERVERS] sys.stdout.write(json.dumps({"service": info, "registry": registry}, indent=2) + "\n") return 0 @@ -91,17 +85,14 @@ def _cmd_status(emit_json: bool) -> int: if broken: out.append(f" broken pairs: {len(broken)}") out += [f" - {b}" for b in broken] - disabled = info.get("disabled_servers") or [] - if disabled: + if disabled := info.get("disabled_servers"): out.append(f" disabled in cfg: {', '.join(disabled)}") - # Sidecar gaps the registry table can't show (bash-language-server -> shellcheck). - backend_warnings = _backend_warnings() - if backend_warnings: + if backend_warnings := _backend_warnings(): out += ["", "Backend warnings", "================"] + [f" ! {line}" for line in backend_warnings] out += ["", "Registered Servers", "=================="] for s in SERVERS: - status = detect_status(_recipe_pkg_for(s.server_id)) + status = _status_for(s.server_id) ext_summary = ", ".join(list(s.extensions)[:5]) if len(s.extensions) > 5: ext_summary += f", … (+{len(s.extensions) - 5})" @@ -114,45 +105,41 @@ def _cmd_status(emit_json: bool) -> int: def _cmd_list(installed_only: bool) -> int: from agent.lsp.servers import SERVERS - from agent.lsp.install import detect_status - for s in SERVERS: - status = detect_status(_recipe_pkg_for(s.server_id)) + status = _status_for(s.server_id) if not (installed_only and status != "installed"): sys.stdout.write(f"{s.server_id:24s} [{status:11s}] {','.join(s.extensions)}\n") return 0 def _cmd_install(server_id: str) -> int: - from agent.lsp.install import try_install, INSTALL_RECIPES, detect_status + from agent.lsp.install import try_install, INSTALL_RECIPES pkg = _recipe_pkg_for(server_id) - if detect_status(pkg) == "installed": + if _status_for(server_id) == "installed": sys.stdout.write(f"{server_id} already installed\n") return 0 sys.stdout.write(f"installing {server_id} (pkg={pkg}) ...\n") sys.stdout.flush() bin_path = try_install(pkg, "auto") - if bin_path is None: - if (INSTALL_RECIPES.get(pkg) or {}).get("strategy") == "manual": - sys.stderr.write(f"{server_id}: this server requires a manual install. See documentation.\n") - else: - sys.stderr.write(f"{server_id}: install failed (see logs).\n") - return 1 - sys.stdout.write(f"installed: {bin_path}\n") - return 0 + if bin_path is not None: + sys.stdout.write(f"installed: {bin_path}\n") + return 0 + manual = (INSTALL_RECIPES.get(pkg) or {}).get("strategy") == "manual" + sys.stderr.write(f"{server_id}: this server requires a manual install. See documentation.\n" if manual + else f"{server_id}: install failed (see logs).\n") + return 1 def _cmd_install_all(include_manual: bool) -> int: from agent.lsp.servers import SERVERS - from agent.lsp.install import try_install, INSTALL_RECIPES, detect_status - + from agent.lsp.install import try_install, INSTALL_RECIPES rc = 0 for s in SERVERS: pkg = _recipe_pkg_for(s.server_id) recipe = INSTALL_RECIPES.get(pkg) if recipe is None or (recipe.get("strategy") == "manual" and not include_manual): continue - if detect_status(pkg) == "installed": + if _status_for(s.server_id) == "installed": sys.stdout.write(f" {s.server_id:24s} already installed\n") continue sys.stdout.write(f" installing {s.server_id} (pkg={pkg}) ... ") @@ -165,7 +152,6 @@ def _cmd_install_all(include_manual: bool) -> int: def _cmd_restart() -> int: from agent.lsp import shutdown_service - shutdown_service() sys.stdout.write("LSP service shut down. Next edit will respawn clients.\n") return 0 @@ -173,7 +159,6 @@ def _cmd_restart() -> int: def _cmd_which(server_id: str) -> int: from agent.lsp.install import INSTALL_RECIPES, _existing_binary - resolved = _existing_binary((INSTALL_RECIPES.get(server_id) or {}).get("bin", server_id)) if resolved: sys.stdout.write(resolved + "\n") diff --git a/agent/lsp/client.py b/agent/lsp/client.py index 86c6231569..efa37dabe6 100644 --- a/agent/lsp/client.py +++ b/agent/lsp/client.py @@ -21,17 +21,8 @@ from urllib.parse import quote, unquote from hermes_cli._subprocess_compat import windows_hide_flags from agent.lsp.protocol import ( - ERROR_CONTENT_MODIFIED, - ERROR_METHOD_NOT_FOUND, - LSPProtocolError, - LSPRequestError, - classify_message, - encode_message, - make_error_response, - make_notification, - make_request, - make_response, - read_message, + ERROR_CONTENT_MODIFIED, ERROR_METHOD_NOT_FOUND, LSPProtocolError, LSPRequestError, classify_message, + encode_message, make_error_response, make_notification, make_request, make_response, read_message, ) logger = logging.getLogger("agent.lsp.client") @@ -102,14 +93,10 @@ def _end_position(text: str) -> Dict[str, int]: @dataclass class _DocState: - """Per-document state. - - ``version`` is the LSP document version last sent (didOpen=0, +1 per didChange) - and doubles as the freshness token: ``push_version`` / ``pull_version`` tag stored - results, fresh iff tag >= version; -1 means "no data yet". Servers that echo a - version in publishDiagnostics get exact tagging; others are credited with the - current version at receipt. - """ + """Per-document state. ``version`` is the LSP document version last sent (didOpen=0, +1 per + didChange) and doubles as the freshness token: ``push_version`` / ``pull_version`` tag stored + results, fresh iff tag >= version; -1 means "no data yet". Servers that echo a version in + publishDiagnostics get exact tagging; others are credited with the current version at receipt.""" version: int = 0 text: str = "" push: List[Dict[str, Any]] = field(default_factory=list) @@ -151,9 +138,8 @@ class LSPClient: self._next_id: int = 0 self._pending: Dict[int, asyncio.Future] = {} - # Server → client requests; anything else gets method-not-found. Capability - # (un)registration and diagnostic refresh are acknowledged but not acted on: - # we re-pull on every touch anyway. + # Server → client requests; anything else gets method-not-found. Capability (un)registration + # and diagnostic refresh are acknowledged but not acted on: we re-pull on every touch anyway. self._request_handlers: Dict[str, Callable[[Any], Awaitable[Any]]] = { "window/workDoneProgress/create": self._handle_null, "workspace/configuration": self._handle_workspace_configuration, @@ -171,8 +157,8 @@ class LSPClient: self._state: str = "stopped" self._sync_kind: int = 1 # 1=Full, 2=Incremental self._stopping: bool = False - # Waiters snapshot ``_push_counter`` and treat any increase as "recheck - # the predicate" — avoids the asyncio.Event sticky-state trap. + # Waiters snapshot ``_push_counter`` and treat any increase as "recheck the + # predicate" — avoids the asyncio.Event sticky-state trap. self._push_event = asyncio.Event() self._push_counter = 0 @@ -216,11 +202,10 @@ class LSPClient: if sys.platform == "win32" and cmd[0].lower().endswith((".cmd", ".bat")): cmd = ["cmd.exe", "/c", *cmd] # CreateProcess can't run .cmd/.bat shims directly try: - # start_new_session=True gives the server its own process group; - # otherwise it inherits the gateway's pgid and mcp_tool's orphan - # sweeper can killpg() the TUI parent along with it. - # windows_hide_flags() suppresses the console window a .cmd shim - # would flash from a console-less host (CREATE_NO_WINDOW; 0 on POSIX). + # start_new_session=True gives the server its own process group; otherwise it inherits + # the gateway's pgid and mcp_tool's orphan sweeper can killpg() the TUI parent with it. + # windows_hide_flags() suppresses the console window a .cmd shim would flash from a + # console-less host (CREATE_NO_WINDOW; 0 on POSIX). self._proc = await asyncio.create_subprocess_exec( cmd[0], *cmd[1:], stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, @@ -243,20 +228,23 @@ class LSPClient: except (asyncio.CancelledError, OSError): pass + def _dispatch(self, msg: dict) -> None: + kind, key = classify_message(msg) + if kind == "response": + self._dispatch_response(key, msg) + elif kind == "request": + asyncio.create_task(self._dispatch_request(key, msg)) + elif kind == "notification": + self._dispatch_notification(key, msg) + else: + logger.warning("[%s] dropping invalid message: %r", self.server_id, msg) + async def _reader_loop(self) -> None: if self._proc is None or self._proc.stdout is None: return try: while (msg := await read_message(self._proc.stdout)) is not None: - kind, key = classify_message(msg) - if kind == "response": - self._dispatch_response(key, msg) - elif kind == "request": - asyncio.create_task(self._dispatch_request(key, msg)) - elif kind == "notification": - self._dispatch_notification(key, msg) - else: - logger.warning("[%s] dropping invalid message: %r", self.server_id, msg) + self._dispatch(msg) logger.debug("[%s] server closed stdout cleanly", self.server_id) except LSPProtocolError as e: logger.warning("[%s] protocol error in reader loop: %s", self.server_id, e) @@ -288,9 +276,7 @@ class LSPClient: sync = sync.get("change") self._sync_kind = sync if isinstance(sync, int) else 1 # default to Full await self._send_notification("initialized", {}) - if self._init_options: - # Some servers (vtsls, eslint) only pick config up via - # didChangeConfiguration even when it was in initializationOptions. + if self._init_options: # vtsls/eslint only pick config up via didChangeConfiguration await self._send_notification("workspace/didChangeConfiguration", {"settings": self._init_options}) async def shutdown(self) -> None: @@ -312,20 +298,15 @@ class LSPClient: self._state = "stopped" await self._cleanup_process() - @staticmethod - async def _cancel_task(task: Optional[asyncio.Task]) -> None: - if task is not None and not task.done(): - task.cancel() - await asyncio.gather(task, return_exceptions=True) - async def _cleanup_process(self) -> None: async with self._cleanup_lock: - reader_task, self._reader_task = self._reader_task, None - stderr_task, self._stderr_task = self._stderr_task, None + tasks = [self._reader_task, self._stderr_task] + self._reader_task = self._stderr_task = None proc, self._proc = self._proc, None - if reader_task is not asyncio.current_task(): - await self._cancel_task(reader_task) - await self._cancel_task(stderr_task) + live = [t for t in tasks if t is not None and not t.done() and t is not asyncio.current_task()] + for t in live: + t.cancel() + await asyncio.gather(*live, return_exceptions=True) if proc is None or proc.returncode is not None: return try: @@ -490,8 +471,7 @@ class LSPClient: "workspace/didChangeWatchedFiles", {"changes": [{"uri": uri, "type": 1 if doc is None else 2}]} ) if doc is None: - # Fresh state: anything a pre-open push stashed under this path - # (relatedDocuments spillover) is discarded. + # Fresh state: anything a pre-open push stashed under this path (relatedDocuments spillover) is discarded. self._docs[abs_path] = _DocState(version=0, text=text) await self._send_notification( "textDocument/didOpen", @@ -520,11 +500,9 @@ class LSPClient: # ---- diagnostics: pull + wait ---- async def _pull_document_diagnostics(self, path: str) -> None: - """Send ``textDocument/diagnostic`` for one file into the pull store. - - Results are tagged with the version captured at send time, so a didChange racing - past the request makes them stale automatically. Silently no-ops on errors. - """ + """Send ``textDocument/diagnostic`` for one file into the pull store. Results are tagged with the + version captured at send time, so a didChange racing past the request makes them stale + automatically. Silently no-ops on errors (server may not support pull).""" abs_path = os.path.abspath(path) doc = self._docs.get(abs_path) sent_version = doc.version if doc else -1 @@ -554,10 +532,9 @@ class LSPClient: timeout: Optional[float] = None) -> bool: """Wait for fresh diagnostics for ``path`` at ``version``; True iff fresh data arrived in budget. - ``mode`` is ``"document"`` (5s) or ``"full"`` (10s); ``timeout`` overrides the - budget (how ``lsp.wait_timeout`` reaches the loop). Callers must treat False - as "no data", NOT "no errors" — the stores may still hold stale entries. - Never throws for servers lacking pull support; the push side still works. + ``mode`` is ``"document"`` (5s) or ``"full"`` (10s); ``timeout`` overrides the budget (how + ``lsp.wait_timeout`` reaches the loop). Callers must treat False as "no data", NOT "no errors" — + the stores may still hold stale entries. Never throws for servers lacking pull support. """ if not (timeout is not None and timeout > 0): timeout = DIAGNOSTICS_FULL_WAIT if mode == "full" else DIAGNOSTICS_DOCUMENT_WAIT @@ -588,9 +565,9 @@ class LSPClient: self._push_event.clear() try: await asyncio.wait_for(self._push_event.wait(), timeout=timeout) - return True except asyncio.TimeoutError: return False + return True async def _wait_for_fresh_push(self, path: str, version: int, timeout: float) -> None: """Wait until a fresh publishDiagnostics arrives for ``path`` at ``version``+.""" @@ -619,11 +596,9 @@ class LSPClient: await self._await_push(min(remaining, 0.5)) def diagnostics_for(self, path: str, *, fresh_only: bool = False) -> List[Dict[str, Any]]: - """Merged + deduped push/pull diagnostics for one file. - - With ``fresh_only=True`` a store only contributes once its version tag has caught - up to the document's — report paths must use this so "stale" and "clean" aren't conflated. - """ + """Merged + deduped push/pull diagnostics for one file. With ``fresh_only=True`` a store only + contributes once its version tag has caught up to the document's — report paths must use this + so "stale" and "clean" aren't conflated.""" doc = self._docs.get(os.path.abspath(path)) if doc is None: return [] @@ -643,12 +618,9 @@ def _dedupe(*lists: List[Dict[str, Any]]) -> List[Dict[str, Any]]: def _diagnostic_key(d: Dict[str, Any]) -> str: - """Content-equality key: severity + code + source + message + range. - - Shared with the manager's cross-edit delta filter (as ``_diag_key``) so both layers - agree on identity. Range is included so an identical error at a second site still - surfaces as new; the manager line-shifts its baseline into post-edit coordinates first. - """ + """Content-equality key: severity + code + source + message + range. Shared with the manager's + cross-edit delta filter (``_diag_key``) so both layers agree on identity. Range is included so an + identical error at a second site still surfaces as new (the manager line-shifts its baseline first).""" rng = d.get("range") or {} start = rng.get("start") or {} end = rng.get("end") or {} diff --git a/agent/lsp/eventlog.py b/agent/lsp/eventlog.py index b105bec9aa..d963c5f948 100644 --- a/agent/lsp/eventlog.py +++ b/agent/lsp/eventlog.py @@ -1,15 +1,12 @@ """Structured logging with steady-state silence for the LSP layer. -LSP fires on every write_file/patch, so the level model keeps ``agent.log`` -greppable (``rg 'lsp\\['``) without noise: DEBUG for steady-state events with -no novel signal (clean, skipped, repeat "no project root" / "server -unavailable"); INFO for once-per-session transitions (first ``active for -``, first ``no project root`` per file) and every diagnostic event; -WARNING for action-required failures (first ``server unavailable`` per -(server_id, binary), every timeout / unexpected error). Dedup uses -module-level sets bounded by the distinct pairs touched in one process — a -bounded LRU was rejected because evicting an entry would re-fire the line -we explicitly want suppressed. +LSP fires on every write_file/patch, so the level model keeps ``agent.log`` greppable +(``rg 'lsp\\['``) without noise: DEBUG for steady-state events with no novel signal (clean, +skipped, repeat "no project root" / "server unavailable"); INFO for once-per-session +transitions (first ``active for ``, first ``no project root`` per file) and every +diagnostic event; WARNING for action-required failures (first ``server unavailable`` per +(server_id, binary), every timeout / unexpected error). Dedup uses module-level sets bounded +by the distinct pairs touched in one process — a bounded LRU would re-fire suppressed lines. """ from __future__ import annotations diff --git a/agent/lsp/install.py b/agent/lsp/install.py index e83df3ace1..bc5046db94 100644 --- a/agent/lsp/install.py +++ b/agent/lsp/install.py @@ -83,15 +83,12 @@ def hermes_lsp_bin_dir() -> Path: def _native_binary_candidates(base: Path) -> list[Path]: """Return platform-native executable candidates for a staged binary (``base`` plus Windows wrappers).""" - candidates = [base] - if _is_windows(): - seen = {str(base).lower()} - for suffix in _WINDOWS_WRAPPER_SUFFIXES: - candidate = Path(str(base) + suffix) - if str(candidate).lower() not in seen: - candidates.append(candidate) - seen.add(str(candidate).lower()) - return candidates + if not _is_windows(): + return [base] + cands: Dict[str, Path] = {} + for c in (base, *(Path(str(base) + s) for s in _WINDOWS_WRAPPER_SUFFIXES)): + cands.setdefault(str(c).lower(), c) + return list(cands.values()) def _first_existing(*bases: Path) -> Optional[Path]: @@ -132,8 +129,7 @@ def _do_install(pkg: str) -> Optional[str]: return shutil.which(pkg) # not in our registry — best-effort: just probe PATH strategy = recipe.get("strategy", "manual") bin_name = recipe.get("bin", pkg) - existing = _existing_binary(bin_name) - if existing: + if existing := _existing_binary(bin_name): return existing if strategy == "manual": logger.debug("[install] %s requires manual install (recipe=%s)", pkg, recipe) diff --git a/agent/lsp/manager.py b/agent/lsp/manager.py index ed23b2beee..9ff23c18ab 100644 --- a/agent/lsp/manager.py +++ b/agent/lsp/manager.py @@ -1,13 +1,11 @@ """Service-level orchestration for LSP clients. :class:`LSPService` bridges the synchronous file_operations layer and the async -:class:`agent.lsp.client.LSPClient`: one asyncio loop in a background thread -(``get_diagnostics_sync`` opens + waits + drains in one blocking call), one lazily -spawned client per ``(server_id, workspace_root)``, a **broken-set** of pairs that -failed to spawn/initialize (never retried for the life of the service), and a -**delta baseline** per file (``snapshot_baseline()`` runs BEFORE a write; the next -``get_diagnostics_sync()`` returns only diagnostics not in it). Off unless config -enables it — file_operations falls back to in-process syntax checks. +:class:`agent.lsp.client.LSPClient`: one asyncio loop in a background thread, one lazily +spawned client per ``(server_id, workspace_root)``, a **broken-set** of pairs that failed +to spawn/initialize (never retried for the life of the service), and a **delta baseline** +per file (``snapshot_baseline()`` runs BEFORE a write; the next ``get_diagnostics_sync()`` +returns only diagnostics not in it). Off unless config enables it. """ from __future__ import annotations @@ -66,8 +64,7 @@ class _BackgroundLoop: if asyncio.iscoroutine(coro): coro.close() raise RuntimeError("background loop not started") - fut = safe_schedule_threadsafe(coro, self._loop) - if fut is None: + if (fut := safe_schedule_threadsafe(coro, self._loop)) is None: raise RuntimeError("background loop not running") try: return fut.result(timeout=timeout) @@ -77,15 +74,15 @@ class _BackgroundLoop: def stop(self) -> None: loop, self._loop = self._loop, None + thread, self._thread = self._thread, None if loop is None: return try: loop.call_soon_threadsafe(loop.stop) except RuntimeError: pass - if self._thread is not None: - self._thread.join(timeout=2.0) - self._thread = None + if thread is not None: + thread.join(timeout=2.0) class LSPService: @@ -135,17 +132,15 @@ class LSPService: except Exception as e: # noqa: BLE001 logger.debug("LSP config load failed: %s", e) return None - lsp_cfg = (cfg.get("lsp") or {}) if isinstance(cfg, dict) else {} - if not isinstance(lsp_cfg, dict): - lsp_cfg = {} + lsp_cfg = cfg.get("lsp") if isinstance(cfg, dict) else None + lsp_cfg = lsp_cfg if isinstance(lsp_cfg, dict) else {} try: idle_timeout = float(lsp_cfg.get("idle_timeout", DEFAULT_IDLE_TIMEOUT)) except (TypeError, ValueError): idle_timeout = DEFAULT_IDLE_TIMEOUT if 0 < idle_timeout < MIN_IDLE_TIMEOUT: - # Below the per-op wait budget the reaper could kill a client mid-flight and - # the outer timeout would then mark the pair broken for the process lifetime. - # Clamp (0 still disables). + # Below the per-op wait budget the reaper could kill a client mid-flight and the outer + # timeout would then mark the pair broken for the process lifetime. Clamp (0 still disables). idle_timeout = MIN_IDLE_TIMEOUT servers_cfg = lsp_cfg.get("servers") or {} servers = {n: c for n, c in servers_cfg.items() if isinstance(c, dict)} if isinstance(servers_cfg, dict) else {} @@ -195,10 +190,7 @@ class LSPService: def snapshot_baseline(self, file_path: str) -> None: """Snapshot current diagnostics for ``file_path`` as the delta baseline (call BEFORE a write). - - Best-effort: failures are swallowed so a flaky server can't break a - write, but outer timeouts mark the pair broken so later edits skip it. - """ + Best-effort: failures are swallowed so a flaky server can't break a write, but they mark the pair broken.""" if not self.enabled_for(file_path): return try: @@ -217,11 +209,10 @@ class LSPService: ) -> _Diags: """Synchronously open ``file_path``, wait for diagnostics, return them. Never raises. - With ``delta`` (default) the result excludes the :meth:`snapshot_baseline`; - ``line_shift`` (from :func:`agent.lsp.range_shift.build_line_shift`) remaps - that baseline into post-edit coordinates first, so pre-existing diagnostics - that merely moved don't look introduced by this edit. ``[]`` when LSP is - disabled, nothing matches, or the server can't be spawned. + With ``delta`` (default) the result excludes the :meth:`snapshot_baseline`; ``line_shift`` (from + :func:`agent.lsp.range_shift.build_line_shift`) remaps that baseline into post-edit coordinates + first, so pre-existing diagnostics that merely moved don't look introduced by this edit. + ``[]`` when LSP is disabled, nothing matches, or the server can't be spawned. """ if not self.enabled_for(file_path): return [] @@ -239,9 +230,9 @@ class LSPService: self._mark_broken_for_file(file_path, e) return [] if diags is None: - # Server alive but no verdict on the post-edit content in budget (common for - # tsserver on big projects). Report "no data" rather than stale stores — that - # would be the ghost-diagnostics bug. Not marked broken: slow is not dead. + # Server alive but no verdict on the post-edit content in budget (common for tsserver on big + # projects). Report "no data" rather than stale stores — that would be the ghost-diagnostics + # bug. Not marked broken: slow is not dead. eventlog.log_timeout(server_id, file_path, kind="fresh diagnostics") return [] if delta: @@ -274,11 +265,9 @@ class LSPService: def _mark_broken_for_file(self, file_path: str, exc: BaseException) -> None: """Mark the file's ``(server_id, root)`` pair broken after an outer timeout/error. - - The outer ``_loop.run`` timeout cancels the in-flight spawn before ``_get_or_spawn`` - could record the failure; without this every later write would re-pay the full - timeout. Also kills any half-initialized client and logs the failure once. - """ + The outer ``_loop.run`` timeout cancels the in-flight spawn before ``_get_or_spawn`` could record + the failure; without this every later write would re-pay the full timeout. Also kills any + half-initialized client and logs the failure once.""" srv = find_server_for_file(file_path) key = self._broken_key(srv, file_path) if srv is not None else None if key is None: @@ -453,8 +442,7 @@ class LSPService: except asyncio.CancelledError: raise except Exception as e: # noqa: BLE001 - # A transient sweep error must not kill the reaper, or the - # unbounded-accumulation leak it exists to fix comes back. + # A transient sweep error must not kill the reaper, or the accumulation leak it fixes comes back. logger.debug("LSP idle reaper sweep error: %s", e) async def _reap_idle_once(self) -> None: @@ -469,8 +457,8 @@ class LSPService: await asyncio.gather(*(client.shutdown() for client in clients), return_exceptions=True) async def _shutdown_async(self) -> None: - reaper, self._idle_reaper_task = self._idle_reaper_task, None - if reaper is not None: + if (reaper := self._idle_reaper_task) is not None: + self._idle_reaper_task = None reaper.cancel() await asyncio.gather(reaper, return_exceptions=True) with self._state_lock: diff --git a/agent/lsp/protocol.py b/agent/lsp/protocol.py index faaca6b194..9596e26b82 100644 --- a/agent/lsp/protocol.py +++ b/agent/lsp/protocol.py @@ -71,8 +71,7 @@ async def _read_headers(reader: asyncio.StreamReader) -> Optional[dict]: async def read_message(reader: asyncio.StreamReader) -> Optional[dict]: """Read one framed message. - Returns ``None`` on clean EOF between messages (typical shutdown); - raises :class:`LSPProtocolError` on malformed framing. + ``None`` on clean EOF between messages (typical shutdown); :class:`LSPProtocolError` on malformed framing. """ headers = await _read_headers(reader) if headers is None: @@ -121,10 +120,8 @@ def make_error_response(req_id: Any, code: int, message: str, data: Any = None) def classify_message(msg: dict) -> Tuple[str, Any]: - """Return ``(kind, key)``: kind ∈ request/response/notification/invalid. - - Key is the id for request/response, the method for notifications, ``None`` for invalid. - """ + """Return ``(kind, key)``: kind ∈ request/response/notification/invalid; key is the id (request/response), + the method (notification) or ``None`` (invalid).""" if not isinstance(msg, dict) or msg.get("jsonrpc") != "2.0": return "invalid", None if "id" in msg: diff --git a/agent/lsp/reporter.py b/agent/lsp/reporter.py index 73f365990c..d942186fe7 100644 --- a/agent/lsp/reporter.py +++ b/agent/lsp/reporter.py @@ -26,10 +26,9 @@ MAX_SOURCE_CHARS = 80 def _sanitize_field(value: Any, *, limit: int) -> str: """Make a language-server field safe to embed in a tool-result block. - ``message``/``code``/``source`` come from a server that just parsed - user-controlled code, so a hostile repo can smuggle instruction-shaped - text through identifier names. We collapse CR/LF, drop control chars, - cap the length, and HTML-escape ``< > &`` so the text can't close + ``message``/``code``/``source`` come from a server that just parsed user-controlled code, so a + hostile repo can smuggle instruction-shaped text through identifier names. We collapse CR/LF, + drop control chars, cap the length, and HTML-escape ``< > &`` so the text can't close ```` early. ``None``/empty → ``""`` so callers can omit the part. """ if value is None: diff --git a/agent/lsp/servers.py b/agent/lsp/servers.py index 232a8d845a..c47016ca90 100644 --- a/agent/lsp/servers.py +++ b/agent/lsp/servers.py @@ -89,7 +89,6 @@ class ServerContext: # ---- helpers ---- - def _file_ext_or_basename(path: str) -> str: """Lower-cased extension, or the full basename for extensionless files (``Dockerfile``).""" base = os.path.basename(path) @@ -148,7 +147,6 @@ def _simple_spawn(server_id: str, which: Sequence[str], args: Sequence[str] = () # ---- bespoke spawn builders ---- - def _spawn_pyright(root: str, ctx: ServerContext) -> Optional[SpawnSpec]: bin_path = _find_binary(ctx, "pyright", ("pyright-langserver", "pyright"), "pyright") if bin_path is None: @@ -173,6 +171,7 @@ _warned_once: set = set() def _warn_once(key: str, message: str) -> None: + """Log ``message`` at WARNING the first time ``key`` is seen in this process.""" if key not in _warned_once: _warned_once.add(key) logger.warning(message) @@ -191,12 +190,9 @@ def _spawn_bash_ls(root: str, ctx: ServerContext) -> Optional[SpawnSpec]: def _find_pses_bundle(ctx: ServerContext) -> Optional[str]: - """Locate the PowerShellEditorServices bundle dir (release zip, manual install). - - Resolution order: ``lsp.servers.powershell.command[0]`` when a directory, - ``init_overrides["powershell"]["bundlePath"]``, ``PSES_BUNDLE_PATH`` env, - then ``/lsp/PowerShellEditorServices``. - """ + """Locate the PowerShellEditorServices bundle dir (release zip, manual install). Resolution order: + ``lsp.servers.powershell.command[0]`` when a directory, ``init_overrides["powershell"]["bundlePath"]``, + ``PSES_BUNDLE_PATH`` env, then ``/lsp/PowerShellEditorServices``.""" from hermes_constants import get_hermes_home override = ctx.binary_overrides.get("powershell") init = ctx.init_overrides.get("powershell", {})