mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-23 16:36:23 +00:00
fix(lsp): never report stale diagnostics — wait for fresh post-edit data
Slow language servers (tsserver on large projects especially) publish
diagnostics long after an edit. The client's wait/report path had three
holes that together surfaced the PREVIOUS edit's errors as if they were
current ("ghost diagnostics"), sending the agent chasing errors it had
already fixed:
1. open_file only cleared the diagnostic stores on first open — on the
didChange path (every subsequent edit) stale push/pull entries
survived.
2. wait_for_diagnostics' predicates were satisfiable by that leftover
state (`path in _published`, `path in _pull_diagnostics`), so the
"wait" often returned instantly with old data.
3. diagnostics_for merged the stale push store unconditionally, so even
a fresh clean pull got the old error merged back in.
Fix: anchor freshness on a per-file didChange timestamp.
- Pull results record their request send-time and are dropped when a
didChange raced past them; the pull store is invalidated on every
change, not just first open.
- wait_for_diagnostics now returns bool (fresh data vs timeout), only
counts pushes published at/after the change (and version >= ours when
the server echoes versions), and accepts an explicit timeout — the
user's lsp.wait_timeout config now actually controls the inner wait
budget instead of only the outer thread-join.
- diagnostics_for(fresh_only=True) excludes stores that predate the
latest change; all manager report paths use it.
- On timeout the manager returns [] ("no data") instead of stale
state, logs a WARNING via eventlog, and does NOT mark the server
broken — slow is not dead.
- seed-on-first-push no longer marks the file published, so the TS
seed push can't satisfy a waiter.
Tests: new "stale" and "slow_push" mock-server scripts model the slow
tsserver, plus client- and service-level regression tests
(tests/agent/lsp/test_stale_diagnostics.py).
This commit is contained in:
parent
41fdcae688
commit
f9b1fd7994
5 changed files with 476 additions and 51 deletions
|
|
@ -198,6 +198,17 @@ class LSPClient:
|
||||||
self._published_version: Dict[str, int] = {}
|
self._published_version: Dict[str, int] = {}
|
||||||
# First-push seen flag, for typescript-style seed-on-first-push.
|
# First-push seen flag, for typescript-style seed-on-first-push.
|
||||||
self._first_push_seen: Set[str] = set()
|
self._first_push_seen: Set[str] = set()
|
||||||
|
# Per-path loop time of the most recent didOpen/didChange we
|
||||||
|
# sent. Freshness anchor: pushes/pulls that predate this are
|
||||||
|
# results for OLD content and must never satisfy a waiter or
|
||||||
|
# be reported to the agent (the "ghost diagnostics" bug —
|
||||||
|
# slow servers like tsserver publish long after the edit, so
|
||||||
|
# leftovers from the previous edit look like current errors).
|
||||||
|
self._changed_at: Dict[str, float] = {}
|
||||||
|
# Per-path loop time at which the most recent *stored* pull
|
||||||
|
# request was sent. Send-time (not receipt) so a didChange
|
||||||
|
# racing a pull in flight correctly invalidates the result.
|
||||||
|
self._pulled_at: Dict[str, float] = {}
|
||||||
# Capability registrations — only diagnostic ones are tracked.
|
# Capability registrations — only diagnostic ones are tracked.
|
||||||
self._diagnostic_registrations: Dict[str, Dict[str, Any]] = {}
|
self._diagnostic_registrations: Dict[str, Dict[str, Any]] = {}
|
||||||
|
|
||||||
|
|
@ -650,15 +661,13 @@ class LSPClient:
|
||||||
loop_time = asyncio.get_event_loop().time()
|
loop_time = asyncio.get_event_loop().time()
|
||||||
|
|
||||||
if self._seed_first_push and path not in self._first_push_seen:
|
if self._seed_first_push and path not in self._first_push_seen:
|
||||||
# First push: seed without firing the event so a waiter
|
# First push: seed the store without marking the file as
|
||||||
# doesn't resolve on the very first push (which arrives
|
# "published" and without firing the event. The very first
|
||||||
# before the user-triggered didChange could've produced
|
# push arrives before the user-triggered didChange could've
|
||||||
# fresh diagnostics).
|
# produced fresh diagnostics, so it must never satisfy a
|
||||||
|
# waiter — it's baseline data only.
|
||||||
self._first_push_seen.add(path)
|
self._first_push_seen.add(path)
|
||||||
self._push_diagnostics[path] = diagnostics
|
self._push_diagnostics[path] = diagnostics
|
||||||
self._published[path] = loop_time
|
|
||||||
if isinstance(version, int):
|
|
||||||
self._published_version[path] = version
|
|
||||||
return
|
return
|
||||||
|
|
||||||
self._push_diagnostics[path] = diagnostics
|
self._push_diagnostics[path] = diagnostics
|
||||||
|
|
@ -725,6 +734,12 @@ class LSPClient:
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
self._files[abs_path] = {"version": new_version, "text": text}
|
self._files[abs_path] = {"version": new_version, "text": text}
|
||||||
|
# Anchor freshness at this change: any pull result stored
|
||||||
|
# before now describes the PREVIOUS content and must not be
|
||||||
|
# reported (it would resurrect just-fixed errors as ghosts).
|
||||||
|
self._changed_at[abs_path] = asyncio.get_event_loop().time()
|
||||||
|
self._pull_diagnostics.pop(abs_path, None)
|
||||||
|
self._pulled_at.pop(abs_path, None)
|
||||||
return new_version
|
return new_version
|
||||||
|
|
||||||
# First open: didChangeWatchedFiles CREATED + didOpen.
|
# First open: didChangeWatchedFiles CREATED + didOpen.
|
||||||
|
|
@ -738,6 +753,8 @@ class LSPClient:
|
||||||
self._pull_diagnostics.pop(abs_path, None)
|
self._pull_diagnostics.pop(abs_path, None)
|
||||||
self._published.pop(abs_path, None)
|
self._published.pop(abs_path, None)
|
||||||
self._published_version.pop(abs_path, None)
|
self._published_version.pop(abs_path, None)
|
||||||
|
self._pulled_at.pop(abs_path, None)
|
||||||
|
self._changed_at[abs_path] = asyncio.get_event_loop().time()
|
||||||
await self._send_notification(
|
await self._send_notification(
|
||||||
"textDocument/didOpen",
|
"textDocument/didOpen",
|
||||||
{
|
{
|
||||||
|
|
@ -771,10 +788,17 @@ class LSPClient:
|
||||||
|
|
||||||
Stores results into :attr:`_pull_diagnostics`. Silently
|
Stores results into :attr:`_pull_diagnostics`. Silently
|
||||||
no-ops on errors (server may not support the pull endpoint).
|
no-ops on errors (server may not support the pull endpoint).
|
||||||
|
|
||||||
|
The request's *send time* is recorded in :attr:`_pulled_at`
|
||||||
|
so freshness checks can compare against :attr:`_changed_at`.
|
||||||
|
A result whose request predates the latest didChange is
|
||||||
|
dropped — it describes content that no longer exists.
|
||||||
"""
|
"""
|
||||||
|
abs_path = os.path.abspath(path)
|
||||||
|
sent_at = asyncio.get_event_loop().time()
|
||||||
try:
|
try:
|
||||||
params: Dict[str, Any] = {
|
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(
|
result = await self._send_request_with_retry(
|
||||||
"textDocument/diagnostic",
|
"textDocument/diagnostic",
|
||||||
|
|
@ -786,9 +810,18 @@ class LSPClient:
|
||||||
return
|
return
|
||||||
if not isinstance(result, dict):
|
if not isinstance(result, dict):
|
||||||
return
|
return
|
||||||
|
if sent_at < self._changed_at.get(abs_path, 0.0):
|
||||||
|
# The document changed while this pull was in flight — the
|
||||||
|
# server answered for the OLD text. Storing it would
|
||||||
|
# resurrect ghost diagnostics.
|
||||||
|
logger.debug(
|
||||||
|
"[%s] dropping stale pull result for %s", self.server_id, abs_path
|
||||||
|
)
|
||||||
|
return
|
||||||
items = result.get("items")
|
items = result.get("items")
|
||||||
if isinstance(items, list):
|
if isinstance(items, list):
|
||||||
self._pull_diagnostics[os.path.abspath(path)] = items
|
self._pull_diagnostics[abs_path] = items
|
||||||
|
self._pulled_at[abs_path] = sent_at
|
||||||
related = result.get("relatedDocuments")
|
related = result.get("relatedDocuments")
|
||||||
if isinstance(related, dict):
|
if isinstance(related, dict):
|
||||||
for uri, sub in related.items():
|
for uri, sub in related.items():
|
||||||
|
|
@ -796,7 +829,36 @@ class LSPClient:
|
||||||
continue
|
continue
|
||||||
sub_items = sub.get("items")
|
sub_items = sub.get("items")
|
||||||
if isinstance(sub_items, list):
|
if isinstance(sub_items, list):
|
||||||
self._pull_diagnostics[uri_to_path(uri)] = sub_items
|
rel_path = uri_to_path(uri)
|
||||||
|
if sent_at < self._changed_at.get(rel_path, 0.0):
|
||||||
|
continue
|
||||||
|
self._pull_diagnostics[rel_path] = sub_items
|
||||||
|
self._pulled_at[rel_path] = sent_at
|
||||||
|
|
||||||
|
def _has_fresh_push(self, path: str, version: int) -> bool:
|
||||||
|
"""True iff a *fresh* publishDiagnostics has arrived for ``path``.
|
||||||
|
|
||||||
|
Fresh means: published at/after the latest didOpen/didChange we
|
||||||
|
sent, AND (when the server echoes versions) for a version >= the
|
||||||
|
one we're waiting on. A stale push left over from the previous
|
||||||
|
edit cycle satisfies neither and must not end a wait early —
|
||||||
|
that's exactly the "ghost diagnostics" failure mode with slow
|
||||||
|
servers like tsserver.
|
||||||
|
"""
|
||||||
|
published_at = self._published.get(path)
|
||||||
|
if published_at is None:
|
||||||
|
return False
|
||||||
|
if published_at < self._changed_at.get(path, 0.0):
|
||||||
|
return False
|
||||||
|
current_v = self._published_version.get(path)
|
||||||
|
return current_v is None or current_v >= version
|
||||||
|
|
||||||
|
def _has_fresh_pull(self, path: str) -> bool:
|
||||||
|
"""True iff the stored pull result postdates the latest change."""
|
||||||
|
pulled_at = self._pulled_at.get(path)
|
||||||
|
if pulled_at is None:
|
||||||
|
return path in self._pull_diagnostics
|
||||||
|
return pulled_at >= self._changed_at.get(path, 0.0)
|
||||||
|
|
||||||
async def wait_for_diagnostics(
|
async def wait_for_diagnostics(
|
||||||
self,
|
self,
|
||||||
|
|
@ -804,22 +866,36 @@ class LSPClient:
|
||||||
version: int,
|
version: int,
|
||||||
*,
|
*,
|
||||||
mode: str = "document",
|
mode: str = "document",
|
||||||
) -> None:
|
timeout: Optional[float] = None,
|
||||||
|
) -> bool:
|
||||||
"""Wait for the server to publish diagnostics for ``path`` at ``version``.
|
"""Wait for the server to publish diagnostics for ``path`` at ``version``.
|
||||||
|
|
||||||
``mode`` is ``"document"`` (5s budget, document pulls) or
|
``mode`` is ``"document"`` (5s budget, document pulls) or
|
||||||
``"full"`` (10s budget, also workspace pulls). Best-effort —
|
``"full"`` (10s budget, also workspace pulls). ``timeout``
|
||||||
returns silently on timeout. Does NOT throw if the server
|
overrides the mode's default budget when provided — this is
|
||||||
doesn't support pull diagnostics; we still get the push side.
|
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
|
deadline = asyncio.get_event_loop().time() + budget
|
||||||
abs_path = os.path.abspath(path)
|
abs_path = os.path.abspath(path)
|
||||||
|
|
||||||
while True:
|
while True:
|
||||||
remaining = deadline - asyncio.get_event_loop().time()
|
remaining = deadline - asyncio.get_event_loop().time()
|
||||||
if remaining <= 0:
|
if remaining <= 0:
|
||||||
return
|
return False
|
||||||
|
|
||||||
# Concurrent: document pull + push wait.
|
# Concurrent: document pull + push wait.
|
||||||
pull_task = asyncio.create_task(self._pull_document_diagnostics(abs_path))
|
pull_task = asyncio.create_task(self._pull_document_diagnostics(abs_path))
|
||||||
|
|
@ -838,26 +914,22 @@ class LSPClient:
|
||||||
pass
|
pass
|
||||||
|
|
||||||
# If we got a fresh push for our version, we're done.
|
# If we got a fresh push for our version, we're done.
|
||||||
current_v = self._published_version.get(abs_path)
|
if self._has_fresh_push(abs_path, version):
|
||||||
if abs_path in self._published and (
|
return True
|
||||||
current_v is None or current_v >= version
|
|
||||||
):
|
|
||||||
return
|
|
||||||
|
|
||||||
# Pull may have populated _pull_diagnostics — that's also
|
# Pull may have populated _pull_diagnostics with a
|
||||||
# success.
|
# post-change answer — that's also success.
|
||||||
if abs_path in self._pull_diagnostics:
|
if self._has_fresh_pull(abs_path):
|
||||||
return
|
return True
|
||||||
|
|
||||||
# Loop until budget runs out.
|
# Loop until budget runs out.
|
||||||
|
|
||||||
async def _wait_for_fresh_push(self, path: str, version: int, timeout: float) -> None:
|
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
|
deadline = asyncio.get_event_loop().time() + timeout
|
||||||
baseline = self._push_counter
|
baseline = self._push_counter
|
||||||
while True:
|
while True:
|
||||||
current_v = self._published_version.get(path)
|
if self._has_fresh_push(path, version):
|
||||||
if path in self._published and (current_v is None or current_v >= version):
|
|
||||||
# Debounce — wait a tick in case more diagnostics arrive
|
# Debounce — wait a tick in case more diagnostics arrive
|
||||||
# immediately after. TS often emits in pairs. We
|
# immediately after. TS often emits in pairs. We
|
||||||
# snapshot the counter so we wake on a *new* push, not
|
# snapshot the counter so we wake on a *new* push, not
|
||||||
|
|
@ -888,14 +960,28 @@ class LSPClient:
|
||||||
except asyncio.TimeoutError:
|
except asyncio.TimeoutError:
|
||||||
continue
|
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.
|
"""Return current merged + deduped diagnostics for one file.
|
||||||
|
|
||||||
Diagnostics from push and pull stores are concatenated and
|
Diagnostics from push and pull stores are concatenated and
|
||||||
deduplicated by ``(severity, code, message, range)`` content
|
deduplicated by ``(severity, code, message, range)`` content
|
||||||
key. Empty list if the server hasn't published anything.
|
key. Empty list if the server hasn't published anything.
|
||||||
|
|
||||||
|
With ``fresh_only=True``, a store only contributes when its
|
||||||
|
data postdates the latest didOpen/didChange for the file —
|
||||||
|
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)
|
abs_path = os.path.abspath(path)
|
||||||
|
if fresh_only:
|
||||||
|
changed_at = self._changed_at.get(abs_path, 0.0)
|
||||||
|
push_fresh = self._published.get(abs_path, -1.0) >= changed_at
|
||||||
|
pulled_at = self._pulled_at.get(abs_path)
|
||||||
|
pull_fresh = pulled_at is not None and pulled_at >= changed_at
|
||||||
|
push = (self._push_diagnostics.get(abs_path) or []) if push_fresh else []
|
||||||
|
pull = (self._pull_diagnostics.get(abs_path) or []) if pull_fresh else []
|
||||||
|
return _dedupe(push, pull)
|
||||||
push = self._push_diagnostics.get(abs_path) or []
|
push = self._push_diagnostics.get(abs_path) or []
|
||||||
pull = self._pull_diagnostics.get(abs_path) or []
|
pull = self._pull_diagnostics.get(abs_path) or []
|
||||||
return _dedupe(push, pull)
|
return _dedupe(push, pull)
|
||||||
|
|
|
||||||
|
|
@ -292,7 +292,10 @@ class LSPService:
|
||||||
if not self.enabled_for(file_path):
|
if not self.enabled_for(file_path):
|
||||||
return
|
return
|
||||||
try:
|
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 []
|
self._delta_baseline[os.path.abspath(file_path)] = diags or []
|
||||||
except Exception as e: # noqa: BLE001
|
except Exception as e: # noqa: BLE001
|
||||||
logger.debug("baseline snapshot failed for %s: %s", file_path, e)
|
logger.debug("baseline snapshot failed for %s: %s", file_path, e)
|
||||||
|
|
@ -341,7 +344,7 @@ class LSPService:
|
||||||
|
|
||||||
try:
|
try:
|
||||||
t = timeout if timeout is not None else self._wait_timeout + 2.0
|
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:
|
except asyncio.TimeoutError as e:
|
||||||
eventlog.log_timeout(server_id, file_path)
|
eventlog.log_timeout(server_id, file_path)
|
||||||
logger.debug("LSP diagnostics timeout for %s: %s", file_path, e)
|
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)
|
self._mark_broken_for_file(file_path, e)
|
||||||
return []
|
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)
|
abs_path = os.path.abspath(file_path)
|
||||||
if delta:
|
if delta:
|
||||||
baseline = self._delta_baseline.get(abs_path) or []
|
baseline = self._delta_baseline.get(abs_path) or []
|
||||||
|
|
@ -452,26 +466,43 @@ class LSPService:
|
||||||
return []
|
return []
|
||||||
try:
|
try:
|
||||||
version = await client.open_file(file_path, language_id=language_id_for(file_path))
|
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
|
except Exception as e: # noqa: BLE001
|
||||||
logger.debug("snapshot open/wait failed: %s", e)
|
logger.debug("snapshot open/wait failed: %s", e)
|
||||||
return []
|
return []
|
||||||
self._last_used[(client.server_id, client.workspace_root)] = time.time()
|
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)
|
client = await self._get_or_spawn(file_path)
|
||||||
if client is None:
|
if client is None:
|
||||||
return []
|
return None
|
||||||
try:
|
try:
|
||||||
version = await client.open_file(file_path, language_id=language_id_for(file_path))
|
version = await client.open_file(file_path, language_id=language_id_for(file_path))
|
||||||
await client.save_file(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
|
except Exception as e: # noqa: BLE001
|
||||||
logger.debug("open/wait failed for %s: %s", file_path, e)
|
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()
|
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]]:
|
async def _current_diags_async(self, file_path: str) -> List[Dict[str, Any]]:
|
||||||
ws, gated = resolve_workspace_for_file(file_path)
|
ws, gated = resolve_workspace_for_file(file_path)
|
||||||
|
|
@ -482,7 +513,7 @@ class LSPService:
|
||||||
client = self._clients.get((srv.server_id, ws))
|
client = self._clients.get((srv.server_id, ws))
|
||||||
if client is None:
|
if client is None:
|
||||||
return []
|
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]:
|
async def _get_or_spawn(self, file_path: str) -> Optional[LSPClient]:
|
||||||
srv = find_server_for_file(file_path)
|
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).
|
(simulates a crashing server).
|
||||||
- ``"slow"`` — same as ``clean`` but sleeps 1s before responding to
|
- ``"slow"`` — same as ``clean`` but sleeps 1s before responding to
|
||||||
``initialize`` (lets us test timeout behaviour).
|
``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
|
The script writes JSON-RPC framed messages to stdout and reads from
|
||||||
stdin. No third-party dependencies — uses only stdlib so it runs
|
stdin. No third-party dependencies — uses only stdlib so it runs
|
||||||
|
|
@ -96,20 +104,47 @@ def main():
|
||||||
td = params.get("textDocument") or {}
|
td = params.get("textDocument") or {}
|
||||||
uri = td.get("uri", "")
|
uri = td.get("uri", "")
|
||||||
version = td.get("version", 0)
|
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 = []
|
diagnostics = []
|
||||||
if script == "errors":
|
if script == "errors":
|
||||||
diagnostics = [
|
diagnostics = 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",
|
|
||||||
}
|
|
||||||
]
|
|
||||||
write_message(
|
write_message(
|
||||||
{
|
{
|
||||||
"jsonrpc": "2.0",
|
"jsonrpc": "2.0",
|
||||||
|
|
@ -124,6 +159,17 @@ def main():
|
||||||
continue
|
continue
|
||||||
|
|
||||||
if msg.get("method") == "textDocument/diagnostic":
|
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.
|
# Pull endpoint — return empty.
|
||||||
write_message(
|
write_message(
|
||||||
{
|
{
|
||||||
|
|
|
||||||
249
tests/agent/lsp/test_stale_diagnostics.py
Normal file
249
tests/agent/lsp/test_stale_diagnostics.py
Normal file
|
|
@ -0,0 +1,249 @@
|
||||||
|
"""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 be stored after a
|
||||||
|
didChange raced past it (send-time 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)
|
||||||
|
assert client._has_fresh_pull(os.path.abspath(str(f)))
|
||||||
|
|
||||||
|
# Simulate an edit racing in: the change invalidates the pull.
|
||||||
|
f.write_text("good code\n")
|
||||||
|
await client.open_file(str(f), language_id="python")
|
||||||
|
assert not client._has_fresh_pull(os.path.abspath(str(f)))
|
||||||
|
assert os.path.abspath(str(f)) not in client._pull_diagnostics
|
||||||
|
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.
|
# How long to wait for diagnostics after each write.
|
||||||
wait_mode: document # "document" or "full"
|
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
|
wait_timeout: 5.0
|
||||||
|
|
||||||
# How to handle missing server binaries.
|
# 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
|
tens of milliseconds for pyright/tsserver and a few seconds for
|
||||||
rust-analyzer mid-indexing.
|
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
|
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
|
no idle-timeout reaper — the cost of restarting the server's index
|
||||||
on every write would be far higher than holding the daemon.
|
on every write would be far higher than holding the daemon.
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue