mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-22 16:25:58 +00:00
Merge pull request #66471 from NousResearch/ethie/typescript-lsp
fix(lsp): never report stale diagnostics — version-gated freshness for slow servers
This commit is contained in:
commit
18ca0e862c
5 changed files with 503 additions and 93 deletions
|
|
@ -18,9 +18,15 @@ into it via :func:`agent.lsp.manager.LSPService.touch_file`.
|
|||
|
||||
Implementation notes:
|
||||
|
||||
- Push diagnostics are stored per-URI in :attr:`_push_diagnostics` from
|
||||
``textDocument/publishDiagnostics`` notifications. Pull diagnostics
|
||||
go in :attr:`_pull_diagnostics`. The merged view dedupes by content.
|
||||
- All per-document state lives in one :class:`_DocState` keyed by
|
||||
absolute path. Freshness is tracked with **document versions**,
|
||||
not timestamps: every didChange bumps ``version``, and each stored
|
||||
push/pull result is tagged with the version it describes. A
|
||||
result is fresh iff its tag >= the version being waited on, so a
|
||||
didChange implicitly invalidates everything older — no clearing,
|
||||
no clock comparisons, no race windows. This is what prevents
|
||||
"ghost diagnostics": a slow server's leftovers from the previous
|
||||
edit can never masquerade as a verdict on the current content.
|
||||
|
||||
- Whole-document sync. Even when the server advertises incremental
|
||||
sync, we send a single ``contentChanges`` entry replacing the
|
||||
|
|
@ -45,6 +51,7 @@ import asyncio
|
|||
import logging
|
||||
import os
|
||||
import sys
|
||||
from dataclasses import dataclass, field
|
||||
from pathlib import Path
|
||||
from typing import Any, Awaitable, Callable, Dict, List, Optional, Set
|
||||
from urllib.parse import quote, unquote
|
||||
|
|
@ -124,6 +131,40 @@ def _end_position(text: str) -> Dict[str, int]:
|
|||
return {"line": last_line, "character": last_col}
|
||||
|
||||
|
||||
@dataclass
|
||||
class _DocState:
|
||||
"""Everything the client tracks for one open document.
|
||||
|
||||
``version`` is the LSP document version we last sent (didOpen=0,
|
||||
each didChange +1). It doubles as the freshness token: stored
|
||||
push/pull results are tagged with the version they describe
|
||||
(``push_version`` / ``pull_version``), and a result is *fresh*
|
||||
iff its tag has caught up to ``version``. Bumping the version on
|
||||
didChange therefore invalidates all older results implicitly —
|
||||
no store-clearing, no timestamps.
|
||||
|
||||
``push_version``/``pull_version`` start at -1 = "no data yet".
|
||||
Servers that echo a document version in publishDiagnostics get
|
||||
exact tagging; those that don't are credited with the current
|
||||
version at receipt time (a push observed after we sent the
|
||||
change describes the changed content or newer).
|
||||
"""
|
||||
|
||||
version: int = 0
|
||||
text: str = ""
|
||||
push: List[Dict[str, Any]] = field(default_factory=list)
|
||||
pull: List[Dict[str, Any]] = field(default_factory=list)
|
||||
push_version: int = -1
|
||||
pull_version: int = -1
|
||||
seed_seen: bool = False
|
||||
|
||||
def fresh_push(self, version: Optional[int] = None) -> bool:
|
||||
return self.push_version >= (self.version if version is None else version)
|
||||
|
||||
def fresh_pull(self, version: Optional[int] = None) -> bool:
|
||||
return self.pull_version >= (self.version if version is None else version)
|
||||
|
||||
|
||||
class LSPClient:
|
||||
"""Async LSP client tied to one server process and one workspace root.
|
||||
|
||||
|
|
@ -186,18 +227,10 @@ class LSPClient:
|
|||
# is silently dropped by default.
|
||||
}
|
||||
|
||||
# Tracked file state — required for didChange version bumps.
|
||||
self._files: Dict[str, Dict[str, Any]] = {}
|
||||
# Diagnostic stores, keyed by file path (NOT URI).
|
||||
self._push_diagnostics: Dict[str, List[Dict[str, Any]]] = {}
|
||||
self._pull_diagnostics: Dict[str, List[Dict[str, Any]]] = {}
|
||||
# Per-path "last published" time so wait-for-fresh logic works.
|
||||
self._published: Dict[str, float] = {}
|
||||
# Per-path version of the latest push (matches our didChange
|
||||
# version when the server respects it).
|
||||
self._published_version: Dict[str, int] = {}
|
||||
# First-push seen flag, for typescript-style seed-on-first-push.
|
||||
self._first_push_seen: Set[str] = set()
|
||||
# Per-document state (version, text, diagnostic stores, and
|
||||
# their freshness tags), keyed by absolute file path (NOT URI).
|
||||
# See _DocState for the version-based freshness model.
|
||||
self._docs: Dict[str, _DocState] = {}
|
||||
# Capability registrations — only diagnostic ones are tracked.
|
||||
self._diagnostic_registrations: Dict[str, Dict[str, Any]] = {}
|
||||
|
||||
|
|
@ -647,25 +680,25 @@ class LSPClient:
|
|||
if not isinstance(diagnostics, list):
|
||||
diagnostics = []
|
||||
version = params.get("version")
|
||||
loop_time = asyncio.get_event_loop().time()
|
||||
|
||||
if self._seed_first_push and path not in self._first_push_seen:
|
||||
# First push: seed without firing the event so a waiter
|
||||
# doesn't resolve on the very first push (which arrives
|
||||
# before the user-triggered didChange could've produced
|
||||
# fresh diagnostics).
|
||||
self._first_push_seen.add(path)
|
||||
self._push_diagnostics[path] = diagnostics
|
||||
self._published[path] = loop_time
|
||||
if isinstance(version, int):
|
||||
self._published_version[path] = version
|
||||
doc = self._docs.setdefault(path, _DocState(version=-1))
|
||||
if self._seed_first_push and not doc.seed_seen:
|
||||
# First push: seed the store WITHOUT a freshness tag. It
|
||||
# arrives before the user-triggered didChange could've
|
||||
# produced fresh diagnostics, so it must never satisfy a
|
||||
# waiter — it's baseline data only.
|
||||
doc.seed_seen = True
|
||||
doc.push = diagnostics
|
||||
return
|
||||
|
||||
self._push_diagnostics[path] = diagnostics
|
||||
self._published[path] = loop_time
|
||||
if isinstance(version, int):
|
||||
self._published_version[path] = version
|
||||
self._first_push_seen.add(path)
|
||||
doc.seed_seen = True
|
||||
doc.push = diagnostics
|
||||
# Tag with the echoed document version when the server provides
|
||||
# one; otherwise credit the current version — a push observed
|
||||
# after we sent the change describes the changed content (or
|
||||
# newer). Note doc.version is -1 for never-opened paths
|
||||
# (e.g. relatedDocuments spillover), keeping them unfresh.
|
||||
doc.push_version = version if isinstance(version, int) else doc.version
|
||||
# Bump the monotonic push counter and wake every waiter. We
|
||||
# keep the Event sticky-set so any wait already in progress
|
||||
# resolves; waiters re-check their predicate after waking and
|
||||
|
|
@ -694,16 +727,16 @@ class LSPClient:
|
|||
raise LSPProtocolError(f"cannot read {abs_path}: {e}") from e
|
||||
|
||||
uri = file_uri(abs_path)
|
||||
existing = self._files.get(abs_path)
|
||||
doc = self._docs.get(abs_path)
|
||||
|
||||
if existing is not None:
|
||||
if doc is not None and doc.version >= 0:
|
||||
# Re-open: bump version, fire didChangeWatchedFiles + didChange.
|
||||
await self._send_notification(
|
||||
"workspace/didChangeWatchedFiles",
|
||||
{"changes": [{"uri": uri, "type": 2}]}, # 2 = CHANGED
|
||||
)
|
||||
new_version = existing["version"] + 1
|
||||
old_text = existing["text"]
|
||||
new_version = doc.version + 1
|
||||
old_text = doc.text
|
||||
content_changes: List[Dict[str, Any]]
|
||||
if self._sync_kind == 2:
|
||||
content_changes = [
|
||||
|
|
@ -724,7 +757,11 @@ class LSPClient:
|
|||
"contentChanges": content_changes,
|
||||
},
|
||||
)
|
||||
self._files[abs_path] = {"version": new_version, "text": text}
|
||||
# Bumping the version is the whole invalidation story:
|
||||
# every stored result tagged with an older version is now
|
||||
# stale by definition (see _DocState).
|
||||
doc.version = new_version
|
||||
doc.text = text
|
||||
return new_version
|
||||
|
||||
# First open: didChangeWatchedFiles CREATED + didOpen.
|
||||
|
|
@ -732,12 +769,9 @@ class LSPClient:
|
|||
"workspace/didChangeWatchedFiles",
|
||||
{"changes": [{"uri": uri, "type": 1}]}, # 1 = CREATED
|
||||
)
|
||||
# Clear any stale push/pull entries — fresh open should start
|
||||
# from scratch.
|
||||
self._push_diagnostics.pop(abs_path, None)
|
||||
self._pull_diagnostics.pop(abs_path, None)
|
||||
self._published.pop(abs_path, None)
|
||||
self._published_version.pop(abs_path, None)
|
||||
# Fresh doc state — anything stashed under this path by a
|
||||
# pre-open push (relatedDocuments spillover etc.) is discarded.
|
||||
self._docs[abs_path] = _DocState(version=0, text=text)
|
||||
await self._send_notification(
|
||||
"textDocument/didOpen",
|
||||
{
|
||||
|
|
@ -749,7 +783,6 @@ class LSPClient:
|
|||
}
|
||||
},
|
||||
)
|
||||
self._files[abs_path] = {"version": 0, "text": text}
|
||||
return 0
|
||||
|
||||
async def save_file(self, path: str) -> None:
|
||||
|
|
@ -769,12 +802,19 @@ class LSPClient:
|
|||
async def _pull_document_diagnostics(self, path: str) -> None:
|
||||
"""Send ``textDocument/diagnostic`` for one file.
|
||||
|
||||
Stores results into :attr:`_pull_diagnostics`. Silently
|
||||
no-ops on errors (server may not support the pull endpoint).
|
||||
Stores results into the doc's pull store, tagged with the
|
||||
document version captured at request send time. If a didChange
|
||||
races past the in-flight request, the version bump makes the
|
||||
stored result stale automatically — no explicit invalidation.
|
||||
Silently no-ops on errors (server may not support the pull
|
||||
endpoint).
|
||||
"""
|
||||
abs_path = os.path.abspath(path)
|
||||
doc = self._docs.get(abs_path)
|
||||
sent_version = doc.version if doc else -1
|
||||
try:
|
||||
params: Dict[str, Any] = {
|
||||
"textDocument": {"uri": file_uri(os.path.abspath(path))}
|
||||
"textDocument": {"uri": file_uri(abs_path)}
|
||||
}
|
||||
result = await self._send_request_with_retry(
|
||||
"textDocument/diagnostic",
|
||||
|
|
@ -788,7 +828,9 @@ class LSPClient:
|
|||
return
|
||||
items = result.get("items")
|
||||
if isinstance(items, list):
|
||||
self._pull_diagnostics[os.path.abspath(path)] = items
|
||||
doc = self._docs.setdefault(abs_path, _DocState(version=-1))
|
||||
doc.pull = items
|
||||
doc.pull_version = sent_version
|
||||
related = result.get("relatedDocuments")
|
||||
if isinstance(related, dict):
|
||||
for uri, sub in related.items():
|
||||
|
|
@ -796,7 +838,11 @@ class LSPClient:
|
|||
continue
|
||||
sub_items = sub.get("items")
|
||||
if isinstance(sub_items, list):
|
||||
self._pull_diagnostics[uri_to_path(uri)] = sub_items
|
||||
rel = self._docs.setdefault(uri_to_path(uri), _DocState(version=-1))
|
||||
rel.pull = sub_items
|
||||
# Same send-anchored tagging: fresh only if that
|
||||
# doc hasn't changed since the request went out.
|
||||
rel.pull_version = rel.version
|
||||
|
||||
async def wait_for_diagnostics(
|
||||
self,
|
||||
|
|
@ -804,22 +850,36 @@ class LSPClient:
|
|||
version: int,
|
||||
*,
|
||||
mode: str = "document",
|
||||
) -> None:
|
||||
timeout: Optional[float] = None,
|
||||
) -> bool:
|
||||
"""Wait for the server to publish diagnostics for ``path`` at ``version``.
|
||||
|
||||
``mode`` is ``"document"`` (5s budget, document pulls) or
|
||||
``"full"`` (10s budget, also workspace pulls). Best-effort —
|
||||
returns silently on timeout. Does NOT throw if the server
|
||||
doesn't support pull diagnostics; we still get the push side.
|
||||
``"full"`` (10s budget, also workspace pulls). ``timeout``
|
||||
overrides the mode's default budget when provided — this is
|
||||
how the user's ``lsp.wait_timeout`` config reaches the wait
|
||||
loop (slow servers like tsserver on big projects need more
|
||||
than the 5s default).
|
||||
|
||||
Returns ``True`` when *fresh* diagnostics arrived (a push at
|
||||
or after our didChange, or a pull answered after it) and
|
||||
``False`` on timeout. Callers must treat ``False`` as "no
|
||||
data", NOT as "no errors" — the diagnostic stores may still
|
||||
hold stale entries from the previous edit at that point.
|
||||
Best-effort — never throws if the server doesn't support pull
|
||||
diagnostics; we still get the push side.
|
||||
"""
|
||||
budget = DIAGNOSTICS_FULL_WAIT if mode == "full" else DIAGNOSTICS_DOCUMENT_WAIT
|
||||
if timeout is not None and timeout > 0:
|
||||
budget = timeout
|
||||
else:
|
||||
budget = DIAGNOSTICS_FULL_WAIT if mode == "full" else DIAGNOSTICS_DOCUMENT_WAIT
|
||||
deadline = asyncio.get_event_loop().time() + budget
|
||||
abs_path = os.path.abspath(path)
|
||||
|
||||
while True:
|
||||
remaining = deadline - asyncio.get_event_loop().time()
|
||||
if remaining <= 0:
|
||||
return
|
||||
return False
|
||||
|
||||
# Concurrent: document pull + push wait.
|
||||
pull_task = asyncio.create_task(self._pull_document_diagnostics(abs_path))
|
||||
|
|
@ -838,26 +898,24 @@ class LSPClient:
|
|||
pass
|
||||
|
||||
# If we got a fresh push for our version, we're done.
|
||||
current_v = self._published_version.get(abs_path)
|
||||
if abs_path in self._published and (
|
||||
current_v is None or current_v >= version
|
||||
):
|
||||
return
|
||||
doc = self._docs.get(abs_path)
|
||||
if doc and doc.fresh_push(version):
|
||||
return True
|
||||
|
||||
# Pull may have populated _pull_diagnostics — that's also
|
||||
# success.
|
||||
if abs_path in self._pull_diagnostics:
|
||||
return
|
||||
# Pull may have answered for the current version — that's
|
||||
# also success.
|
||||
if doc and doc.fresh_pull(version):
|
||||
return True
|
||||
|
||||
# Loop until budget runs out.
|
||||
|
||||
async def _wait_for_fresh_push(self, path: str, version: int, timeout: float) -> None:
|
||||
"""Wait until a publishDiagnostics arrives for ``path`` at ``version``+."""
|
||||
"""Wait until a fresh publishDiagnostics arrives for ``path`` at ``version``+."""
|
||||
deadline = asyncio.get_event_loop().time() + timeout
|
||||
baseline = self._push_counter
|
||||
while True:
|
||||
current_v = self._published_version.get(path)
|
||||
if path in self._published and (current_v is None or current_v >= version):
|
||||
doc = self._docs.get(path)
|
||||
if doc and doc.fresh_push(version):
|
||||
# Debounce — wait a tick in case more diagnostics arrive
|
||||
# immediately after. TS often emits in pairs. We
|
||||
# snapshot the counter so we wake on a *new* push, not
|
||||
|
|
@ -888,17 +946,28 @@ class LSPClient:
|
|||
except asyncio.TimeoutError:
|
||||
continue
|
||||
|
||||
def diagnostics_for(self, path: str) -> List[Dict[str, Any]]:
|
||||
def diagnostics_for(self, path: str, *, fresh_only: bool = False) -> List[Dict[str, Any]]:
|
||||
"""Return current merged + deduped diagnostics for one file.
|
||||
|
||||
Diagnostics from push and pull stores are concatenated and
|
||||
deduplicated by ``(severity, code, message, range)`` content
|
||||
key. Empty list if the server hasn't published anything.
|
||||
|
||||
With ``fresh_only=True``, a store only contributes when its
|
||||
version tag has caught up to the document's current version —
|
||||
stale leftovers from the previous edit cycle are excluded.
|
||||
This is what report paths should use: after an edit, "stale
|
||||
errors" and "no errors" must not be conflated.
|
||||
"""
|
||||
abs_path = os.path.abspath(path)
|
||||
push = self._push_diagnostics.get(abs_path) or []
|
||||
pull = self._pull_diagnostics.get(abs_path) or []
|
||||
return _dedupe(push, pull)
|
||||
doc = self._docs.get(os.path.abspath(path))
|
||||
if doc is None:
|
||||
return []
|
||||
if fresh_only:
|
||||
return _dedupe(
|
||||
doc.push if doc.fresh_push() else [],
|
||||
doc.pull if doc.fresh_pull() else [],
|
||||
)
|
||||
return _dedupe(doc.push, doc.pull)
|
||||
|
||||
|
||||
def _dedupe(*lists: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
|
||||
|
|
|
|||
|
|
@ -292,7 +292,10 @@ class LSPService:
|
|||
if not self.enabled_for(file_path):
|
||||
return
|
||||
try:
|
||||
diags = self._loop.run(self._snapshot_async(file_path), timeout=8.0)
|
||||
# Outer join budget must exceed the inner wait budget or a
|
||||
# slow-but-alive server gets falsely marked broken.
|
||||
t = max(8.0, self._wait_timeout + 3.0)
|
||||
diags = self._loop.run(self._snapshot_async(file_path), timeout=t)
|
||||
self._delta_baseline[os.path.abspath(file_path)] = diags or []
|
||||
except Exception as e: # noqa: BLE001
|
||||
logger.debug("baseline snapshot failed for %s: %s", file_path, e)
|
||||
|
|
@ -341,7 +344,7 @@ class LSPService:
|
|||
|
||||
try:
|
||||
t = timeout if timeout is not None else self._wait_timeout + 2.0
|
||||
diags = self._loop.run(self._open_and_wait_async(file_path), timeout=t) or []
|
||||
diags = self._loop.run(self._open_and_wait_async(file_path), timeout=t)
|
||||
except asyncio.TimeoutError as e:
|
||||
eventlog.log_timeout(server_id, file_path)
|
||||
logger.debug("LSP diagnostics timeout for %s: %s", file_path, e)
|
||||
|
|
@ -353,6 +356,17 @@ class LSPService:
|
|||
self._mark_broken_for_file(file_path, e)
|
||||
return []
|
||||
|
||||
if diags is None:
|
||||
# The server is alive but never produced diagnostics for the
|
||||
# post-edit content within the wait budget (common for
|
||||
# tsserver on large projects). Report "no data" rather than
|
||||
# whatever stale state is in the stores — surfacing the
|
||||
# previous edit's errors as if they were current is the
|
||||
# ghost-diagnostics bug. The server is NOT marked broken:
|
||||
# slow is not dead, and the next edit may well succeed.
|
||||
eventlog.log_timeout(server_id, file_path, kind="fresh diagnostics")
|
||||
return []
|
||||
|
||||
abs_path = os.path.abspath(file_path)
|
||||
if delta:
|
||||
baseline = self._delta_baseline.get(abs_path) or []
|
||||
|
|
@ -452,26 +466,43 @@ class LSPService:
|
|||
return []
|
||||
try:
|
||||
version = await client.open_file(file_path, language_id=language_id_for(file_path))
|
||||
await client.wait_for_diagnostics(file_path, version, mode=self._wait_mode)
|
||||
fresh = await client.wait_for_diagnostics(file_path, version, mode=self._wait_mode)
|
||||
except Exception as e: # noqa: BLE001
|
||||
logger.debug("snapshot open/wait failed: %s", e)
|
||||
return []
|
||||
self._last_used[(client.server_id, client.workspace_root)] = time.time()
|
||||
return list(client.diagnostics_for(file_path))
|
||||
if not fresh:
|
||||
# No fresh data for the pre-edit content — an empty baseline
|
||||
# is safe: worst case the delta filter removes less, never
|
||||
# more. Never seed the baseline from stale stores.
|
||||
return []
|
||||
return list(client.diagnostics_for(file_path, fresh_only=True))
|
||||
|
||||
async def _open_and_wait_async(self, file_path: str) -> List[Dict[str, Any]]:
|
||||
async def _open_and_wait_async(self, file_path: str) -> Optional[List[Dict[str, Any]]]:
|
||||
"""Open + wait for FRESH diagnostics.
|
||||
|
||||
Returns the fresh diagnostic list, or ``None`` when the server
|
||||
never produced post-change data within the wait budget. The
|
||||
distinction matters: ``[]`` means "server checked the new
|
||||
content, it's clean", ``None`` means "no verdict" — the caller
|
||||
must not substitute stale data for either.
|
||||
"""
|
||||
client = await self._get_or_spawn(file_path)
|
||||
if client is None:
|
||||
return []
|
||||
return None
|
||||
try:
|
||||
version = await client.open_file(file_path, language_id=language_id_for(file_path))
|
||||
await client.save_file(file_path)
|
||||
await client.wait_for_diagnostics(file_path, version, mode=self._wait_mode)
|
||||
fresh = await client.wait_for_diagnostics(
|
||||
file_path, version, mode=self._wait_mode, timeout=self._wait_timeout
|
||||
)
|
||||
except Exception as e: # noqa: BLE001
|
||||
logger.debug("open/wait failed for %s: %s", file_path, e)
|
||||
return []
|
||||
return None
|
||||
self._last_used[(client.server_id, client.workspace_root)] = time.time()
|
||||
return list(client.diagnostics_for(file_path))
|
||||
if not fresh:
|
||||
return None
|
||||
return list(client.diagnostics_for(file_path, fresh_only=True))
|
||||
|
||||
async def _current_diags_async(self, file_path: str) -> List[Dict[str, Any]]:
|
||||
ws, gated = resolve_workspace_for_file(file_path)
|
||||
|
|
@ -482,7 +513,7 @@ class LSPService:
|
|||
client = self._clients.get((srv.server_id, ws))
|
||||
if client is None:
|
||||
return []
|
||||
return list(client.diagnostics_for(file_path))
|
||||
return list(client.diagnostics_for(file_path, fresh_only=True))
|
||||
|
||||
async def _get_or_spawn(self, file_path: str) -> Optional[LSPClient]:
|
||||
srv = find_server_for_file(file_path)
|
||||
|
|
|
|||
|
|
@ -17,6 +17,14 @@ Behaviour (all behaviours selectable via env var ``MOCK_LSP_SCRIPT``):
|
|||
(simulates a crashing server).
|
||||
- ``"slow"`` — same as ``clean`` but sleeps 1s before responding to
|
||||
``initialize`` (lets us test timeout behaviour).
|
||||
- ``"stale"`` — pushes one error on ``didOpen``, then goes SILENT on
|
||||
``didChange`` (no push) and rejects the pull endpoint with
|
||||
method-not-found. Models a slow tsserver that hasn't re-checked
|
||||
the edited content yet — the ghost-diagnostics scenario.
|
||||
- ``"slow_push"`` — like ``stale`` on didOpen (one error) but on
|
||||
``didChange`` sleeps ``MOCK_LSP_PUSH_DELAY`` seconds (default 1.0)
|
||||
and then pushes EMPTY diagnostics. Models a server that fixes
|
||||
the ghost if you actually wait for it. Pull endpoint rejects.
|
||||
|
||||
The script writes JSON-RPC framed messages to stdout and reads from
|
||||
stdin. No third-party dependencies — uses only stdlib so it runs
|
||||
|
|
@ -96,20 +104,47 @@ def main():
|
|||
td = params.get("textDocument") or {}
|
||||
uri = td.get("uri", "")
|
||||
version = td.get("version", 0)
|
||||
is_change = msg.get("method") == "textDocument/didChange"
|
||||
error_diag = [
|
||||
{
|
||||
"range": {
|
||||
"start": {"line": 0, "character": 0},
|
||||
"end": {"line": 0, "character": 5},
|
||||
},
|
||||
"severity": 1,
|
||||
"code": "MOCK001",
|
||||
"source": "mock-lsp",
|
||||
"message": "synthetic error from mock-lsp",
|
||||
}
|
||||
]
|
||||
if script == "stale":
|
||||
# Ghost scenario: publish an error for the ORIGINAL
|
||||
# content, then never publish again after edits.
|
||||
if not is_change:
|
||||
write_message(
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"method": "textDocument/publishDiagnostics",
|
||||
"params": {"uri": uri, "version": version, "diagnostics": error_diag},
|
||||
}
|
||||
)
|
||||
continue
|
||||
if script == "slow_push":
|
||||
diagnostics = error_diag
|
||||
if is_change:
|
||||
time.sleep(float(os.environ.get("MOCK_LSP_PUSH_DELAY", "1.0")))
|
||||
diagnostics = []
|
||||
write_message(
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"method": "textDocument/publishDiagnostics",
|
||||
"params": {"uri": uri, "version": version, "diagnostics": diagnostics},
|
||||
}
|
||||
)
|
||||
continue
|
||||
diagnostics = []
|
||||
if script == "errors":
|
||||
diagnostics = [
|
||||
{
|
||||
"range": {
|
||||
"start": {"line": 0, "character": 0},
|
||||
"end": {"line": 0, "character": 5},
|
||||
},
|
||||
"severity": 1,
|
||||
"code": "MOCK001",
|
||||
"source": "mock-lsp",
|
||||
"message": "synthetic error from mock-lsp",
|
||||
}
|
||||
]
|
||||
diagnostics = error_diag
|
||||
write_message(
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
|
|
@ -124,6 +159,17 @@ def main():
|
|||
continue
|
||||
|
||||
if msg.get("method") == "textDocument/diagnostic":
|
||||
if script in {"stale", "slow_push"}:
|
||||
# These scripts model push-only servers so the ghost
|
||||
# can't be papered over by the pull channel.
|
||||
write_message(
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": msg["id"],
|
||||
"error": {"code": -32601, "message": "method not found"},
|
||||
}
|
||||
)
|
||||
continue
|
||||
# Pull endpoint — return empty.
|
||||
write_message(
|
||||
{
|
||||
|
|
|
|||
251
tests/agent/lsp/test_stale_diagnostics.py
Normal file
251
tests/agent/lsp/test_stale_diagnostics.py
Normal file
|
|
@ -0,0 +1,251 @@
|
|||
"""Regression tests for the "ghost diagnostics" staleness bug.
|
||||
|
||||
Scenario: the agent edits a TypeScript file, tsserver takes a long
|
||||
time to re-check it, and the old diagnostics (for the PRE-edit
|
||||
content) were reported as if they were current — the agent then
|
||||
chases errors it already fixed.
|
||||
|
||||
The contract under test:
|
||||
|
||||
- ``wait_for_diagnostics`` must NOT be satisfied by diagnostics left
|
||||
over from a previous edit cycle; it returns True only when fresh
|
||||
(post-didChange) data arrived, False on timeout.
|
||||
- ``diagnostics_for(fresh_only=True)`` must exclude stale stores.
|
||||
- ``LSPService.get_diagnostics_sync`` must return [] ("no data")
|
||||
rather than the stale diagnostics when the server never re-checks
|
||||
within the wait budget, and must NOT mark the server broken.
|
||||
- A slow-but-eventually-correct server ("slow_push") is waited on,
|
||||
honouring the configured ``lsp.wait_timeout``.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from agent.lsp.client import LSPClient
|
||||
|
||||
|
||||
MOCK_SERVER = str(Path(__file__).parent / "_mock_lsp_server.py")
|
||||
|
||||
|
||||
def _client(workspace: Path, script: str, **env_extra: str) -> LSPClient:
|
||||
env = {
|
||||
"MOCK_LSP_SCRIPT": script,
|
||||
"PYTHONPATH": os.environ.get("PYTHONPATH", ""),
|
||||
**env_extra,
|
||||
}
|
||||
return LSPClient(
|
||||
server_id=f"mock-{script}",
|
||||
workspace_root=str(workspace),
|
||||
command=[sys.executable, MOCK_SERVER],
|
||||
env=env,
|
||||
cwd=str(workspace),
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_stale_push_does_not_satisfy_wait(tmp_path: Path):
|
||||
"""A push from the previous edit cycle must not end the wait early.
|
||||
|
||||
The 'stale' mock publishes an error for the original content and
|
||||
then goes silent — the wait after the edit must time out (False),
|
||||
not return instantly on the leftover push.
|
||||
"""
|
||||
f = tmp_path / "x.py"
|
||||
f.write_text("bad code\n")
|
||||
|
||||
client = _client(tmp_path, "stale")
|
||||
await client.start()
|
||||
try:
|
||||
v0 = await client.open_file(str(f), language_id="python")
|
||||
assert await client.wait_for_diagnostics(str(f), v0, mode="document", timeout=2.0)
|
||||
assert len(client.diagnostics_for(str(f))) == 1 # pre-edit error is real
|
||||
|
||||
# Fix the file. The stale server never re-checks.
|
||||
f.write_text("good code\n")
|
||||
v1 = await client.open_file(str(f), language_id="python")
|
||||
fresh = await client.wait_for_diagnostics(str(f), v1, mode="document", timeout=1.0)
|
||||
assert fresh is False, "wait must not be satisfied by pre-edit leftovers"
|
||||
finally:
|
||||
await client.shutdown()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fresh_only_excludes_stale_stores(tmp_path: Path):
|
||||
f = tmp_path / "x.py"
|
||||
f.write_text("bad code\n")
|
||||
|
||||
client = _client(tmp_path, "stale")
|
||||
await client.start()
|
||||
try:
|
||||
v0 = await client.open_file(str(f), language_id="python")
|
||||
await client.wait_for_diagnostics(str(f), v0, mode="document", timeout=2.0)
|
||||
|
||||
f.write_text("good code\n")
|
||||
await client.open_file(str(f), language_id="python")
|
||||
# Merged legacy view still exposes the leftover push...
|
||||
assert len(client.diagnostics_for(str(f))) == 1
|
||||
# ...but the fresh-only view correctly reports no verdict yet.
|
||||
assert client.diagnostics_for(str(f), fresh_only=True) == []
|
||||
finally:
|
||||
await client.shutdown()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_slow_push_is_waited_for(tmp_path: Path):
|
||||
"""A server that re-checks slowly (but within budget) gets waited on,
|
||||
and the fresh (clean) result replaces the old error."""
|
||||
f = tmp_path / "x.py"
|
||||
f.write_text("bad code\n")
|
||||
|
||||
client = _client(tmp_path, "slow_push", MOCK_LSP_PUSH_DELAY="0.8")
|
||||
await client.start()
|
||||
try:
|
||||
v0 = await client.open_file(str(f), language_id="python")
|
||||
assert await client.wait_for_diagnostics(str(f), v0, mode="document", timeout=2.0)
|
||||
assert len(client.diagnostics_for(str(f), fresh_only=True)) == 1
|
||||
|
||||
f.write_text("good code\n")
|
||||
v1 = await client.open_file(str(f), language_id="python")
|
||||
fresh = await client.wait_for_diagnostics(str(f), v1, mode="document", timeout=5.0)
|
||||
assert fresh is True, "slow push within budget must satisfy the wait"
|
||||
assert client.diagnostics_for(str(f), fresh_only=True) == []
|
||||
finally:
|
||||
await client.shutdown()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_wait_timeout_param_overrides_mode_budget(tmp_path: Path):
|
||||
"""The explicit timeout must control the wait budget (config plumb)."""
|
||||
import asyncio
|
||||
|
||||
f = tmp_path / "x.py"
|
||||
f.write_text("bad code\n")
|
||||
|
||||
client = _client(tmp_path, "stale")
|
||||
await client.start()
|
||||
try:
|
||||
v0 = await client.open_file(str(f), language_id="python")
|
||||
await client.wait_for_diagnostics(str(f), v0, mode="document", timeout=2.0)
|
||||
f.write_text("good code\n")
|
||||
v1 = await client.open_file(str(f), language_id="python")
|
||||
|
||||
loop = asyncio.get_event_loop()
|
||||
start = loop.time()
|
||||
fresh = await client.wait_for_diagnostics(str(f), v1, mode="document", timeout=0.5)
|
||||
elapsed = loop.time() - start
|
||||
assert fresh is False
|
||||
# Must respect ~0.5s, not the 5s document default.
|
||||
assert elapsed < 3.0
|
||||
finally:
|
||||
await client.shutdown()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_stale_pull_result_dropped_when_change_races(tmp_path: Path):
|
||||
"""A pull answered for pre-edit content must not read as fresh after
|
||||
a didChange raced past it (version-tag anchoring)."""
|
||||
f = tmp_path / "x.py"
|
||||
f.write_text("bad code\n")
|
||||
|
||||
client = _client(tmp_path, "clean")
|
||||
await client.start()
|
||||
try:
|
||||
v0 = await client.open_file(str(f), language_id="python")
|
||||
await client.wait_for_diagnostics(str(f), v0, mode="document", timeout=2.0)
|
||||
doc = client._docs[os.path.abspath(str(f))]
|
||||
assert doc.fresh_pull()
|
||||
|
||||
# Simulate an edit racing in: the version bump invalidates the
|
||||
# stored pull without any explicit clearing.
|
||||
f.write_text("good code\n")
|
||||
await client.open_file(str(f), language_id="python")
|
||||
assert not doc.fresh_pull()
|
||||
assert client.diagnostics_for(str(f), fresh_only=True) == []
|
||||
finally:
|
||||
await client.shutdown()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Service-level: stale data must surface as "no data", never as errors
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _install_mock_server(script: str, server_id: str = "pyright"):
|
||||
"""Replace one registered server with a wrapper spawning the mock.
|
||||
|
||||
Mirrors the helper in test_service.py — reuse pyright so .py files
|
||||
route to the mock without a real toolchain.
|
||||
"""
|
||||
from agent.lsp.servers import SERVERS, ServerContext, ServerDef, SpawnSpec
|
||||
|
||||
target_index = next(i for i, s in enumerate(SERVERS) if s.server_id == server_id)
|
||||
original = SERVERS[target_index]
|
||||
|
||||
def _spawn(root: str, ctx: ServerContext) -> SpawnSpec:
|
||||
return SpawnSpec(
|
||||
command=[sys.executable, MOCK_SERVER],
|
||||
workspace_root=root,
|
||||
cwd=root,
|
||||
env={"MOCK_LSP_SCRIPT": script},
|
||||
initialization_options={},
|
||||
)
|
||||
|
||||
SERVERS[target_index] = ServerDef(
|
||||
server_id=server_id,
|
||||
extensions=original.extensions,
|
||||
resolve_root=lambda fp, ws: ws,
|
||||
build_spawn=_spawn,
|
||||
seed_first_push=False,
|
||||
description="mock " + server_id,
|
||||
)
|
||||
return target_index, original
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def stale_repo(monkeypatch, tmp_path):
|
||||
repo = tmp_path / "repo"
|
||||
repo.mkdir()
|
||||
(repo / ".git").mkdir()
|
||||
(repo / "pyproject.toml").write_text("")
|
||||
monkeypatch.chdir(str(repo))
|
||||
idx, original = _install_mock_server("stale")
|
||||
yield repo
|
||||
from agent.lsp.servers import SERVERS
|
||||
|
||||
SERVERS[idx] = original
|
||||
|
||||
|
||||
def test_service_reports_no_data_not_stale_errors(stale_repo):
|
||||
"""When the server never re-checks the edited content in budget,
|
||||
get_diagnostics_sync must return [] and keep the server usable."""
|
||||
from agent.lsp.manager import LSPService
|
||||
|
||||
f = stale_repo / "x.py"
|
||||
f.write_text("bad code\n")
|
||||
|
||||
svc = LSPService(
|
||||
enabled=True,
|
||||
wait_mode="document",
|
||||
wait_timeout=1.0,
|
||||
install_strategy="manual",
|
||||
)
|
||||
try:
|
||||
# First contact: didOpen gets the (real) pre-edit error push.
|
||||
first = svc.get_diagnostics_sync(str(f), delta=False)
|
||||
assert len(first) == 1
|
||||
|
||||
# Edit the file — mock never re-publishes (slow tsserver model).
|
||||
f.write_text("good code\n")
|
||||
ghost = svc.get_diagnostics_sync(str(f), delta=False)
|
||||
assert ghost == [], "stale pre-edit error must not be reported as current"
|
||||
|
||||
# Not marked broken: slow is not dead.
|
||||
assert svc.enabled_for(str(f))
|
||||
status = svc.get_status()
|
||||
assert status["broken"] == []
|
||||
finally:
|
||||
svc.shutdown()
|
||||
|
|
@ -151,6 +151,12 @@ lsp:
|
|||
|
||||
# How long to wait for diagnostics after each write.
|
||||
wait_mode: document # "document" or "full"
|
||||
# Max seconds to wait for the server to re-check the file after an
|
||||
# edit. Only *fresh* diagnostics (produced for the post-edit
|
||||
# content) are ever reported; if the server doesn't finish within
|
||||
# this budget, the edit reports "no LSP data" rather than stale
|
||||
# errors from before the edit. Raise this for slow servers on big
|
||||
# projects (tsserver, rust-analyzer mid-indexing).
|
||||
wait_timeout: 5.0
|
||||
|
||||
# How to handle missing server binaries.
|
||||
|
|
@ -209,6 +215,13 @@ budget is `wait_timeout` seconds — typically the server responds in
|
|||
tens of milliseconds for pyright/tsserver and a few seconds for
|
||||
rust-analyzer mid-indexing.
|
||||
|
||||
Diagnostics are **freshness-gated**: a result only counts when the
|
||||
server produced it for the content of the current edit (a
|
||||
`publishDiagnostics` push at/after the change, or a pull request
|
||||
answered after it). Slow servers that haven't re-checked yet result
|
||||
in "no data" for that edit — never in yesterday's errors being
|
||||
re-reported as current.
|
||||
|
||||
Servers are kept alive for the life of the Hermes process. There's
|
||||
no idle-timeout reaper — the cost of restarting the server's index
|
||||
on every write would be far higher than holding the daemon.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue