PR #24467: consolidation: fix 7 defects in PR #24168 LSP feature (orphan procs, patch_replace double-lint, opt-in defaults, +4 more)

• Repository: NousResearch/hermes-agent
• Author: scubamount
• State: closed | merged: False
• Link: https://github.com/NousResearch/hermes-agent/pull/24467

Description (problem / solution / changelog)

Consolidation of PR #24168 (LSP semantic diagnostics) — 7 defect fixes

Builds on top of #24168 HEAD (7fa5657). Supersedes the competing #24155 (simpler/smaller approach to the same feature). Addresses every issue raised in the #24168 review thread plus three additional defects found during a holistic re-review.

Context

PR #24168 adds a substantial LSP integration (~4200 LOC across 2 commits) — full agent/lsp/ module spawning real language servers (pyright, gopls, rust-analyzer, etc.) as subprocesses, feeding semantic diagnostics into write_file/patch_replace post-write lint. Architecture is sound; the issues below are wiring/lifecycle/defaults bugs that would bite production users.

Defects fixed

Fix summary

• D1: Token-keyed _delta_baseline (UUID per snapshot). patch_replace captures its own token before the write, passes it to write_file via _lsp_baseline_token=, and the inner _check_lint_delta runs with skip_lsp=True so the token survives to the outer post-write check.
• D2: Same token-keyed map. Each snapshot returns a fresh token; no implicit global state for concurrent writers to collide on.
• D3: Background _reaper_loop coroutine scheduled on _BackgroundLoop. _reap_idle() is the unit-testable single pass.
• D4: OrderedDict LRU (cap 1024) + threading.Lock. Added public invalidate_workspace_cache(path) + clear_workspace_cache() for callers observing workspace topology changes (git init, worktree add/remove).
• D5: Defaults flipped to lsp.enabled: false + install_strategy: "manual". Both opt-in. Existing-binary discovery still works in manual mode (servers on PATH).
• D6: Every auto-install recipe pinned to a specific verified version. Inline CVE notes for [email protected] (CVE-2026-33532, CVSS 4.3 low, DoS-only on deeply-nested YAML, server is sandboxed) and gopls v0.21.1 (CVE-2026-42503, CVSS 8.8 high — exploit gated on -listen/-port flags our default invocation doesn't pass).
• D7: atexit.register(shutdown_service) on first successful get_service() + explicit shutdown_service() call wired into cli._run_cleanup. Belt + suspenders. shutdown_service is idempotent and swallows inner exceptions so an exit hook can never crash the process.

Test coverage

• 204/204 unit tests pass (26 new regression tests in tests/agent/lsp/test_consolidation_regressions.py — one per defect, plus thread-safety + idempotency + ordering pins).
• Live verification of D7: spawned a child Python process that imported the LSP service, opened a real pyright subprocess (PID 77138), and exited normally. After exit, no orphan pyright process survived. Atexit hook fired correctly.

Diffstat

agent/lsp/__init__.py                              |  24 +-
agent/lsp/install.py                               |  61 +-
agent/lsp/manager.py                               | 230 ++++++-
agent/lsp/workspace.py                             | 110 ++-
cli.py                                             |  10 +
hermes_cli/config.py                               |  29 +-
tests/agent/lsp/test_consolidation_regressions.py  | 902 ++++++++++++++++++++  (new)
tests/agent/lsp/test_service.py                    |  11 +-
tools/file_operations.py                           | 107 ++-
9 files changed, 1400 insertions(+), 84 deletions(-)

#24168 bot review cross-reference

Open items, non-blocking

• CVE notes inline for yaml-language-server and gopls — both bounded by deployment posture (sandboxed YAML parsing, no flags). Track upstream patches separately.
• Pre-existing race on _service singleton in get_service() not introduced by this PR; low probability in practice (single-threaded tool dispatch in hermes main loop).

Verifying locally

git fetch https://github.com/scubamount/hermes-agent.git lsp-consolidation-pr24168-fixes
git checkout FETCH_HEAD
python -m pytest tests/agent/lsp/ tests/tools/test_file_operations.py \
                 tests/tools/test_file_operations_edge_cases.py \
                 tests/tools/test_file_write_safety.py

Expected: 204 passed.

cc @teknium1 (PR #24168 author) — this is positioned as a friendly consolidation, not a takeover; happy to close in favor of you pulling these patches into #24168 directly, or to keep this PR live as a stacked target. Whichever you prefer.

Changed files

• agent/lsp/__init__.py (added, +83/-0)
• agent/lsp/cli.py (added, +270/-0)
• agent/lsp/client.py (added, +930/-0)
• agent/lsp/eventlog.py (added, +213/-0)
• agent/lsp/install.py (added, +382/-0)
• agent/lsp/manager.py (added, +692/-0)
• agent/lsp/protocol.py (added, +196/-0)
• agent/lsp/reporter.py (added, +78/-0)
• agent/lsp/servers.py (added, +1025/-0)
• agent/lsp/workspace.py (added, +323/-0)
• cli.py (modified, +10/-0)
• hermes_cli/config.py (modified, +64/-0)
• hermes_cli/main.py (modified, +11/-0)
• tests/agent/lsp/__init__.py (added, +1/-0)
• tests/agent/lsp/_mock_lsp_server.py (added, +159/-0)
• tests/agent/lsp/test_backend_gate.py (added, +108/-0)
• tests/agent/lsp/test_client_e2e.py (added, +143/-0)
• tests/agent/lsp/test_consolidation_regressions.py (added, +902/-0)
• tests/agent/lsp/test_eventlog.py (added, +199/-0)
• tests/agent/lsp/test_protocol.py (added, +197/-0)
• tests/agent/lsp/test_reporter.py (added, +94/-0)
• tests/agent/lsp/test_service.py (added, +152/-0)
• tests/agent/lsp/test_workspace.py (added, +139/-0)
• tools/file_operations.py (modified, +200/-21)
• website/docs/reference/cli-commands.md (modified, +28/-0)
• website/docs/user-guide/features/lsp.md (added, +223/-0)
• website/sidebars.ts (modified, +1/-0)

PR #25037: fix(lsp): default to opt-in (enabled=False, install_strategy=manual) (#25015)

• Repository: NousResearch/hermes-agent
• Author: briandevans
• State: open | merged: False
• Link: https://github.com/NousResearch/hermes-agent/pull/25037

Description (problem / solution / changelog)

Summary

Flips the two canonical LSP defaults in DEFAULT_CONFIG["lsp"] to opt-in so a fresh install never silently spawns pyright / gopls / tsserver or auto-installs language servers via npm / go / pip on the first file edit.

• enabled: True → False
• install_strategy: "auto" → "manual"

Also flips the matching fallbacks in LSPService.create_from_config() so users on a config.yaml that predates the lsp: section see the same opt-in behaviour at runtime.

Fixes #25015.

The bug

hermes_cli/config.py:1516,1530 shipped with enabled: True + install_strategy: "auto". With LSP gated only on git-workspace detection, the first .py / .go / .rs / .ts edit inside any git repo silently triggers:

• npm install -g pyright@latest (or go install gopls@latest, cargo install rust-analyzer, etc.) into <HERMES_HOME>/lsp/bin/
• a long-lived language-server subprocess (pyright ~200 MB, gopls ~80 MB, tsserver ~150 MB)

For users in audited environments (SOC 2, ISO 27001) this falls under the same compliance umbrella that already gates pip / npm installs in CI: any background process or package fetch needs explicit consent.

Reported as defect D5 in scubamount's #24467 and verified-real on current main by @kshitijk4poor when that PR was split into three focused issues. This is the standalone follow-up they asked for.

The fix

Two literal flips in the canonical defaults, plus matching fallback flips in the consumer so existing configs behave the same way:

 # hermes_cli/config.py
 "lsp": {
-    "enabled": True,
+    "enabled": False,
-    "install_strategy": "auto",
+    "install_strategy": "manual",
 }

 # agent/lsp/manager.py — LSPService.create_from_config
-enabled = bool(lsp_cfg.get("enabled", True))
-install_strategy = lsp_cfg.get("install_strategy", "auto")
+enabled = bool(lsp_cfg.get("enabled", False))
+install_strategy = lsp_cfg.get("install_strategy", "manual")

Why also flip the manager fallbacks: LSPService.create_from_config() defaults are what kicks in when config.yaml predates the lsp: section. Flipping only DEFAULT_CONFIG would mean fresh installs get opt-in defaults while existing users silently keep enabled=True / install_strategy="auto" — divergent behaviour for the same product state, and the worst of both worlds.

Why "manual" and not "off": "manual" preserves existing-binary discovery. A user who opts back in with enabled: true (without also opting in to auto-install) still gets diagnostics from any pyright / gopls / tsserver already on PATH, with a clear "not installed" error otherwise. "off" would be a stronger downgrade that punishes users who already have the binaries.

Test plan

• Focused regression test (TestLSPOptInDefaults, 3 tests): asserts DEFAULT_CONFIG["lsp"]["enabled"] is False, install_strategy == "manual", and that the manager fallbacks match.
• Adjacent suite: tests/hermes_cli/test_config.py + tests/agent/lsp/ → 169 passed locally.
• Regression guard: confirmed in-process that the old defaults (True / "auto") would fail the new assertions, and the new defaults pass.

Related

• Source analysis: scubamount's closed #24467 (defect D5).
• Sibling issues filed alongside #25015 by the same split:
  • #25016 — idle-subprocess reaper (already in flight as luyao618's #25021).
  • #25017 — INSTALL_RECIPES @latest pinning.

Sibling code paths that may need the same conservative-default treatment: agent/lsp/servers.py INSTALL_RECIPES (currently pulls @latest, #25017). Intentionally left out of this PR's scope to keep the diff focused on the opt-in toggle — happy to widen if preferred.

Changed files

• agent/lsp/manager.py (modified, +6/-2)
• hermes_cli/config.py (modified, +16/-6)
• tests/hermes_cli/test_config.py (modified, +33/-0)