mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-18 04:41:56 +00:00
fix(lsp): shift baseline diagnostics into post-edit coordinates (#25978)
Pre-existing diagnostics below an edit point used to surface as 'LSP diagnostics introduced by this edit' whenever the edit deleted or inserted lines. The delta-filter key included the diagnostic's range, so the same logical error reported at a different line in the post-edit snapshot looked like a brand new diagnostic. Concrete case: deleting 14 lines in cli.py caused Pyright errors at lines 9873, 10590, 12413, 13004 (unrelated to the edit) to be reported as introduced by it. Fix: build a piecewise-linear line-shift map (via difflib's SequenceMatcher) from pre and post content, and remap baseline diagnostics into post-edit coordinates before the set-difference. Diagnostics in deleted regions drop out cleanly; diagnostics below the edit shift by the right amount; diagnostics above are untouched. The strict (range-aware) equality key stays — so a genuinely new instance of an identical error class at a different line still surfaces as new. Pieces: - agent/lsp/range_shift.py — build_line_shift, shift_diagnostic_range, shift_baseline. Pure functions, no LSP state. - agent/lsp/manager.py — LSPService.get_diagnostics_sync gains an optional line_shift kwarg; baseline is shift_baseline'd before computing the seen-set. _diag_key keeps the strict range key. - tools/file_operations.py — write_file captures pre_content for any LSP-handled extension (not just LINTERS_INPROC) and passes pre/post to _maybe_lsp_diagnostics, which builds the shift map. - New _lsp_handles_extension helper guards the pre_content read. Trade-offs preserved: - Genuinely new same-class errors at different lines still surface (content-only key would have swallowed them). - Pre-existing errors at unshifted positions still get filtered (covered by the strict-key path with no shift). - Best-effort: when pre_content can't be captured (file didn't exist, permissions), the unshifted comparison still catches most pre-existing errors; the edge case it misses is a new file with a non-empty baseline, which is structurally impossible.
This commit is contained in:
parent
ed84637d11
commit
19071529f6
5 changed files with 552 additions and 18 deletions
|
|
@ -40,7 +40,7 @@ import os
|
||||||
import threading
|
import threading
|
||||||
import time
|
import time
|
||||||
from concurrent.futures import Future as ConcurrentFuture
|
from concurrent.futures import Future as ConcurrentFuture
|
||||||
from typing import Any, Dict, List, Optional, Tuple
|
from typing import Any, Callable, Dict, List, Optional, Tuple
|
||||||
|
|
||||||
from agent.lsp import eventlog
|
from agent.lsp import eventlog
|
||||||
from agent.lsp.client import (
|
from agent.lsp.client import (
|
||||||
|
|
@ -305,6 +305,7 @@ class LSPService:
|
||||||
*,
|
*,
|
||||||
delta: bool = True,
|
delta: bool = True,
|
||||||
timeout: Optional[float] = None,
|
timeout: Optional[float] = None,
|
||||||
|
line_shift: Optional[Callable[[int], Optional[int]]] = None,
|
||||||
) -> List[Dict[str, Any]]:
|
) -> List[Dict[str, Any]]:
|
||||||
"""Synchronously open ``file_path`` in the right server, wait for
|
"""Synchronously open ``file_path`` in the right server, wait for
|
||||||
diagnostics, return them.
|
diagnostics, return them.
|
||||||
|
|
@ -314,6 +315,18 @@ class LSPService:
|
||||||
Diagnostics present in the baseline are removed so the caller
|
Diagnostics present in the baseline are removed so the caller
|
||||||
only sees errors introduced by the current edit.
|
only sees errors introduced by the current edit.
|
||||||
|
|
||||||
|
When ``line_shift`` is provided, baseline diagnostics are
|
||||||
|
remapped through it before the set-difference. This handles
|
||||||
|
the case where the edit deleted or inserted lines, causing
|
||||||
|
pre-existing diagnostics below the edit point to surface at
|
||||||
|
different line numbers in the post-edit snapshot — without
|
||||||
|
the shift, they'd all look "introduced by this edit". Pass
|
||||||
|
a callable built by
|
||||||
|
:func:`agent.lsp.range_shift.build_line_shift` (pre_text,
|
||||||
|
post_text). Omit when pre/post content isn't available;
|
||||||
|
the unshifted comparison still catches diagnostics that
|
||||||
|
didn't move.
|
||||||
|
|
||||||
Returns an empty list when LSP is disabled, when no workspace
|
Returns an empty list when LSP is disabled, when no workspace
|
||||||
can be detected, when no server matches, or when the server
|
can be detected, when no server matches, or when the server
|
||||||
can't be spawned. Never raises.
|
can't be spawned. Never raises.
|
||||||
|
|
@ -344,6 +357,14 @@ class LSPService:
|
||||||
if delta:
|
if delta:
|
||||||
baseline = self._delta_baseline.get(abs_path) or []
|
baseline = self._delta_baseline.get(abs_path) or []
|
||||||
if baseline:
|
if baseline:
|
||||||
|
if line_shift is not None:
|
||||||
|
# Remap baseline diagnostics into post-edit
|
||||||
|
# coordinates so shifted-but-otherwise-identical
|
||||||
|
# entries hash equal under _diag_key. Entries
|
||||||
|
# that mapped into a deleted region drop out
|
||||||
|
# silently — they no longer apply.
|
||||||
|
from agent.lsp.range_shift import shift_baseline
|
||||||
|
baseline = shift_baseline(baseline, line_shift)
|
||||||
seen = {_diag_key(d) for d in baseline}
|
seen = {_diag_key(d) for d in baseline}
|
||||||
diags = [d for d in diags if _diag_key(d) not in seen]
|
diags = [d for d in diags if _diag_key(d) not in seen]
|
||||||
# Roll baseline forward — next call returns deltas relative
|
# Roll baseline forward — next call returns deltas relative
|
||||||
|
|
@ -585,8 +606,19 @@ class LSPService:
|
||||||
|
|
||||||
|
|
||||||
def _diag_key(d: Dict[str, Any]) -> str:
|
def _diag_key(d: Dict[str, Any]) -> str:
|
||||||
"""Content equality key used for delta filtering. Mirrors
|
"""Content equality key used for cross-edit delta filtering.
|
||||||
:func:`agent.lsp.client._diagnostic_key`."""
|
|
||||||
|
Includes the diagnostic's position range — when used together
|
||||||
|
with :func:`agent.lsp.range_shift.shift_baseline`, the baseline
|
||||||
|
is line-shifted into post-edit coordinates BEFORE this key is
|
||||||
|
computed, so identical-but-shifted diagnostics hash equal. Two
|
||||||
|
genuinely distinct diagnostics at different lines (e.g. the same
|
||||||
|
error class introduced at a second site) hash differently and
|
||||||
|
are surfaced as new.
|
||||||
|
|
||||||
|
Mirrors :func:`agent.lsp.client._diagnostic_key`; intentionally
|
||||||
|
identical so the two layers agree on diagnostic identity.
|
||||||
|
"""
|
||||||
rng = d.get("range") or {}
|
rng = d.get("range") or {}
|
||||||
start = rng.get("start") or {}
|
start = rng.get("start") or {}
|
||||||
end = rng.get("end") or {}
|
end = rng.get("end") or {}
|
||||||
|
|
|
||||||
149
agent/lsp/range_shift.py
Normal file
149
agent/lsp/range_shift.py
Normal file
|
|
@ -0,0 +1,149 @@
|
||||||
|
"""Diff-aware line-shift map for cross-edit LSP delta filtering.
|
||||||
|
|
||||||
|
When an edit deletes or inserts lines in the middle of a file, every
|
||||||
|
diagnostic below the edit point shifts to a new line number. The
|
||||||
|
LSPService delta filter subtracts the pre-edit baseline from the
|
||||||
|
post-edit diagnostics keyed on ``(severity, code, source, message,
|
||||||
|
range)`` — without an adjustment, the shifted-but-otherwise-identical
|
||||||
|
diagnostics look brand-new and the agent gets flooded with noise.
|
||||||
|
|
||||||
|
The fix used here is the same trick git's blame and unified diff use:
|
||||||
|
build a piecewise-linear map from pre-edit line numbers to post-edit
|
||||||
|
line numbers, then apply that map to baseline diagnostics before the
|
||||||
|
set-difference. Diagnostics whose pre-edit line is in a region the
|
||||||
|
edit deleted return ``None`` and are dropped from the baseline (they
|
||||||
|
genuinely no longer apply).
|
||||||
|
|
||||||
|
Trade-off vs. dropping range from the key entirely (the previous
|
||||||
|
fix): preserves the "new instance of an identical error at a
|
||||||
|
different line" signal — if the model introduces a second instance
|
||||||
|
of the same error class at a different location, that one will be
|
||||||
|
surfaced as new instead of swallowed by content-only dedup.
|
||||||
|
|
||||||
|
The map is derived from ``difflib.SequenceMatcher.get_opcodes()`` and
|
||||||
|
exposed as a single callable so callers don't have to reason about
|
||||||
|
diff regions.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import difflib
|
||||||
|
from typing import Any, Callable, Dict, List, Optional
|
||||||
|
|
||||||
|
|
||||||
|
def build_line_shift(pre_text: str, post_text: str) -> Callable[[int], Optional[int]]:
|
||||||
|
"""Build a function mapping pre-edit line numbers to post-edit line numbers.
|
||||||
|
|
||||||
|
Lines are 0-indexed to match the LSP wire format
|
||||||
|
(``range.start.line`` is 0-indexed).
|
||||||
|
|
||||||
|
The returned callable takes a pre-edit 0-indexed line number and
|
||||||
|
returns the corresponding post-edit 0-indexed line number, or
|
||||||
|
``None`` if that line was deleted by the edit (no post-edit
|
||||||
|
counterpart exists).
|
||||||
|
|
||||||
|
Cost: one ``SequenceMatcher.get_opcodes()`` call up front; the
|
||||||
|
returned closure is O(log n) per call (binary search over opcode
|
||||||
|
regions). Cheap enough to call once per write/patch and apply to
|
||||||
|
every baseline diagnostic.
|
||||||
|
"""
|
||||||
|
pre_lines = pre_text.splitlines() if pre_text else []
|
||||||
|
post_lines = post_text.splitlines() if post_text else []
|
||||||
|
|
||||||
|
# Trivial case: identical content or no content — identity map.
|
||||||
|
if pre_lines == post_lines:
|
||||||
|
return lambda line: line
|
||||||
|
|
||||||
|
# SequenceMatcher.get_opcodes() returns a list of
|
||||||
|
# (tag, i1, i2, j1, j2) where tag is 'equal', 'replace', 'delete',
|
||||||
|
# or 'insert'. i1:i2 is the range in pre, j1:j2 is the range in
|
||||||
|
# post. We build a list of (i1, i2, j1, j2, tag) tuples and
|
||||||
|
# binary-search by i for each lookup.
|
||||||
|
sm = difflib.SequenceMatcher(a=pre_lines, b=post_lines, autojunk=False)
|
||||||
|
opcodes = sm.get_opcodes()
|
||||||
|
|
||||||
|
def shift(line: int) -> Optional[int]:
|
||||||
|
# Find the opcode region whose i1 <= line < i2.
|
||||||
|
# Linear scan is fine — typical opcode count is small (single
|
||||||
|
# digits for a typical patch-tool edit).
|
||||||
|
for tag, i1, i2, j1, j2 in opcodes:
|
||||||
|
if i1 <= line < i2:
|
||||||
|
if tag == "equal":
|
||||||
|
# Pre-line N → post-line (N - i1 + j1).
|
||||||
|
return line - i1 + j1
|
||||||
|
if tag == "delete":
|
||||||
|
# Pre-line is in a deleted region — no post counterpart.
|
||||||
|
return None
|
||||||
|
if tag == "replace":
|
||||||
|
# Replace == delete + insert; the pre-line has no
|
||||||
|
# post counterpart in any meaningful sense. Drop.
|
||||||
|
return None
|
||||||
|
# 'insert' has i1 == i2 so line < i2 can't be hit.
|
||||||
|
if line < i1:
|
||||||
|
# Past the relevant region — handled in earlier iteration.
|
||||||
|
break
|
||||||
|
# Past the last opcode region (line >= len(pre_lines)).
|
||||||
|
# Anchor at end of post.
|
||||||
|
return max(0, len(post_lines) - 1) if post_lines else None
|
||||||
|
|
||||||
|
return shift
|
||||||
|
|
||||||
|
|
||||||
|
def shift_diagnostic_range(diag: Dict[str, Any],
|
||||||
|
shift: Callable[[int], Optional[int]]) -> Optional[Dict[str, Any]]:
|
||||||
|
"""Return a copy of ``diag`` with its line range remapped through ``shift``.
|
||||||
|
|
||||||
|
Returns ``None`` if the diagnostic's start line maps to ``None``
|
||||||
|
(the line was deleted by the edit) — caller drops it from the
|
||||||
|
baseline since the diagnostic no longer applies.
|
||||||
|
|
||||||
|
Both ``start.line`` and ``end.line`` are remapped independently;
|
||||||
|
when only the end maps to ``None`` (rare, multi-line diagnostic
|
||||||
|
straddling the edit boundary) we collapse to a single-line range
|
||||||
|
at the shifted start to keep the diagnostic in the baseline.
|
||||||
|
|
||||||
|
The original ``diag`` is not mutated.
|
||||||
|
"""
|
||||||
|
rng = diag.get("range") or {}
|
||||||
|
start = rng.get("start") or {}
|
||||||
|
end = rng.get("end") or {}
|
||||||
|
|
||||||
|
pre_start_line = int(start.get("line", 0))
|
||||||
|
pre_end_line = int(end.get("line", pre_start_line))
|
||||||
|
|
||||||
|
new_start_line = shift(pre_start_line)
|
||||||
|
if new_start_line is None:
|
||||||
|
return None
|
||||||
|
|
||||||
|
new_end_line = shift(pre_end_line)
|
||||||
|
if new_end_line is None:
|
||||||
|
# Diagnostic straddled the deletion — collapse to start.
|
||||||
|
new_end_line = new_start_line
|
||||||
|
|
||||||
|
shifted = dict(diag)
|
||||||
|
shifted["range"] = {
|
||||||
|
"start": {
|
||||||
|
"line": new_start_line,
|
||||||
|
"character": int(start.get("character", 0)),
|
||||||
|
},
|
||||||
|
"end": {
|
||||||
|
"line": new_end_line,
|
||||||
|
"character": int(end.get("character", 0)),
|
||||||
|
},
|
||||||
|
}
|
||||||
|
return shifted
|
||||||
|
|
||||||
|
|
||||||
|
def shift_baseline(baseline: List[Dict[str, Any]],
|
||||||
|
shift: Callable[[int], Optional[int]]) -> List[Dict[str, Any]]:
|
||||||
|
"""Apply ``shift`` to every diagnostic in ``baseline``, dropping deleted entries."""
|
||||||
|
out: List[Dict[str, Any]] = []
|
||||||
|
for d in baseline:
|
||||||
|
if not isinstance(d, dict):
|
||||||
|
continue
|
||||||
|
shifted = shift_diagnostic_range(d, shift)
|
||||||
|
if shifted is not None:
|
||||||
|
out.append(shifted)
|
||||||
|
return out
|
||||||
|
|
||||||
|
|
||||||
|
__all__ = ["build_line_shift", "shift_diagnostic_range", "shift_baseline"]
|
||||||
262
tests/agent/lsp/test_delta_key.py
Normal file
262
tests/agent/lsp/test_delta_key.py
Normal file
|
|
@ -0,0 +1,262 @@
|
||||||
|
"""Tests for cross-edit LSP delta filtering.
|
||||||
|
|
||||||
|
The delta-filter contract spans three pieces:
|
||||||
|
|
||||||
|
1. ``agent.lsp.manager._diag_key`` — strict equality key including
|
||||||
|
the diagnostic's position range. Two diagnostics with the same
|
||||||
|
content but different lines are NOT equal under this key (they
|
||||||
|
are genuinely different diagnostics).
|
||||||
|
2. ``agent.lsp.range_shift.build_line_shift`` — derives a function
|
||||||
|
mapping pre-edit line numbers to post-edit line numbers from a
|
||||||
|
pre/post text pair.
|
||||||
|
3. ``agent.lsp.manager.LSPService.get_diagnostics_sync(line_shift=…)``
|
||||||
|
— applies the shift to baseline diagnostics before computing the
|
||||||
|
set-difference, so pre-existing errors at shifted lines hash
|
||||||
|
equal to their post-edit counterparts and get filtered out.
|
||||||
|
|
||||||
|
These tests exercise the contract at the unit level; the E2E case
|
||||||
|
(real LSP server, real shift) is covered in test_service.py.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from agent.lsp.client import _diagnostic_key
|
||||||
|
from agent.lsp.manager import _diag_key
|
||||||
|
from agent.lsp.range_shift import (
|
||||||
|
build_line_shift,
|
||||||
|
shift_baseline,
|
||||||
|
shift_diagnostic_range,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _diag(*, line: int, message: str = "Undefined variable",
|
||||||
|
severity: int = 1, code: str = "reportUndefinedVariable",
|
||||||
|
source: str = "Pyright", end_line: int | None = None) -> dict:
|
||||||
|
if end_line is None:
|
||||||
|
end_line = line
|
||||||
|
return {
|
||||||
|
"severity": severity,
|
||||||
|
"code": code,
|
||||||
|
"source": source,
|
||||||
|
"message": message,
|
||||||
|
"range": {
|
||||||
|
"start": {"line": line, "character": 0},
|
||||||
|
"end": {"line": end_line, "character": 10},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
# _diag_key: strict equality (with range)
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
|
||||||
|
def test_diag_key_treats_shifted_diagnostics_as_distinct():
|
||||||
|
"""Two diagnostics with the same message but at different lines hash
|
||||||
|
differently — they are genuinely different diagnostics. The shift
|
||||||
|
map is what makes them equal AFTER remapping; the key itself stays
|
||||||
|
strict."""
|
||||||
|
a = _diag(line=100)
|
||||||
|
b = _diag(line=200)
|
||||||
|
assert _diag_key(a) != _diag_key(b)
|
||||||
|
|
||||||
|
|
||||||
|
def test_diag_key_matches_client_key_for_shifted_baseline():
|
||||||
|
"""When a baseline diagnostic is remapped through a shift, its
|
||||||
|
_diag_key must match the corresponding post-edit diagnostic's key
|
||||||
|
at the same coordinates. This is the contract the delta filter
|
||||||
|
relies on."""
|
||||||
|
pre = _diag(line=200)
|
||||||
|
# Edit deletes 14 lines above line 200, so the same error now
|
||||||
|
# appears at line 186 post-edit.
|
||||||
|
shift = lambda L: L - 14 if L >= 14 else L
|
||||||
|
shifted = shift_diagnostic_range(pre, shift)
|
||||||
|
assert shifted is not None
|
||||||
|
post = _diag(line=186)
|
||||||
|
assert _diag_key(shifted) == _diag_key(post)
|
||||||
|
|
||||||
|
|
||||||
|
def test_diag_key_distinguishes_message():
|
||||||
|
a = _diag(line=100, message="foo")
|
||||||
|
b = _diag(line=100, message="bar")
|
||||||
|
assert _diag_key(a) != _diag_key(b)
|
||||||
|
|
||||||
|
|
||||||
|
def test_diag_key_distinguishes_severity():
|
||||||
|
a = _diag(line=100, severity=1)
|
||||||
|
b = _diag(line=100, severity=2)
|
||||||
|
assert _diag_key(a) != _diag_key(b)
|
||||||
|
|
||||||
|
|
||||||
|
def test_diag_key_distinguishes_source():
|
||||||
|
a = _diag(line=100, source="Pyright")
|
||||||
|
b = _diag(line=100, source="Ruff")
|
||||||
|
assert _diag_key(a) != _diag_key(b)
|
||||||
|
|
||||||
|
|
||||||
|
def test_diag_key_matches_client_key_byte_for_byte():
|
||||||
|
"""The manager-side and client-side keys must agree on diagnostic
|
||||||
|
identity — they're used by two layers that need to round-trip the
|
||||||
|
same diagnostics through dedup and delta filtering."""
|
||||||
|
d = _diag(line=42)
|
||||||
|
assert _diag_key(d) == _diagnostic_key(d)
|
||||||
|
|
||||||
|
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
# build_line_shift
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
|
||||||
|
def test_shift_identity_for_identical_content():
|
||||||
|
shift = build_line_shift("a\nb\nc\n", "a\nb\nc\n")
|
||||||
|
assert shift(0) == 0
|
||||||
|
assert shift(1) == 1
|
||||||
|
assert shift(2) == 2
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_pure_deletion_above_line():
|
||||||
|
"""Delete 2 lines at the top; everything below shifts up by 2."""
|
||||||
|
pre = "line0\nline1\nline2\nline3\nline4\n"
|
||||||
|
post = "line2\nline3\nline4\n" # deleted lines 0-1
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
# Pre lines 0,1 → deleted → None
|
||||||
|
assert shift(0) is None
|
||||||
|
assert shift(1) is None
|
||||||
|
# Pre line 2 → post line 0
|
||||||
|
assert shift(2) == 0
|
||||||
|
# Pre line 4 → post line 2
|
||||||
|
assert shift(4) == 2
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_pure_insertion_above_line():
|
||||||
|
"""Insert 3 lines at the top; everything below shifts down by 3."""
|
||||||
|
pre = "line0\nline1\nline2\n"
|
||||||
|
post = "new0\nnew1\nnew2\nline0\nline1\nline2\n"
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
# Pre lines unchanged in identity, shifted by 3
|
||||||
|
assert shift(0) == 3
|
||||||
|
assert shift(1) == 4
|
||||||
|
assert shift(2) == 5
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_replacement_in_middle():
|
||||||
|
"""Replace 2 lines in the middle with 1 line. Lines above
|
||||||
|
unchanged; lines below shift up by 1."""
|
||||||
|
pre = "a\nb\nc\nd\ne\n"
|
||||||
|
post = "a\nb\nX\ne\n" # replaced lines 2,3 (c,d) with X
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
assert shift(0) == 0 # a → a
|
||||||
|
assert shift(1) == 1 # b → b
|
||||||
|
assert shift(2) is None # c → deleted
|
||||||
|
assert shift(3) is None # d → deleted
|
||||||
|
assert shift(4) == 3 # e → post line 3
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_handles_empty_pre():
|
||||||
|
"""First write of a file: pre is empty, post has content. Nothing
|
||||||
|
to shift, so the function should be well-defined for empty pre."""
|
||||||
|
shift = build_line_shift("", "hello\nworld\n")
|
||||||
|
# Any pre line falls past the end of an empty pre — anchor at end of post
|
||||||
|
assert shift(0) == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_handles_empty_post():
|
||||||
|
"""File deleted to empty. Every pre line returns None."""
|
||||||
|
shift = build_line_shift("line0\nline1\n", "")
|
||||||
|
assert shift(0) is None
|
||||||
|
assert shift(1) is None
|
||||||
|
|
||||||
|
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
# shift_diagnostic_range
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
|
||||||
|
def test_shift_diag_remaps_start_and_end():
|
||||||
|
pre = "a\nb\nc\nd\n"
|
||||||
|
post = "X\na\nb\nc\nd\n" # one line inserted at top
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
d = _diag(line=2, end_line=2)
|
||||||
|
remapped = shift_diagnostic_range(d, shift)
|
||||||
|
assert remapped is not None
|
||||||
|
assert remapped["range"]["start"]["line"] == 3
|
||||||
|
assert remapped["range"]["end"]["line"] == 3
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_diag_drops_diagnostic_in_deleted_region():
|
||||||
|
pre = "a\nb\nc\nd\n"
|
||||||
|
post = "a\nd\n" # deleted lines 1,2 (b,c)
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
d = _diag(line=1)
|
||||||
|
assert shift_diagnostic_range(d, shift) is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_diag_does_not_mutate_original():
|
||||||
|
pre = "a\nb\n"
|
||||||
|
post = "X\na\nb\n"
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
d = _diag(line=0)
|
||||||
|
original_line = d["range"]["start"]["line"]
|
||||||
|
_ = shift_diagnostic_range(d, shift)
|
||||||
|
assert d["range"]["start"]["line"] == original_line
|
||||||
|
|
||||||
|
|
||||||
|
def test_shift_baseline_drops_deleted_and_remaps_rest():
|
||||||
|
pre = "a\nb\nc\nd\ne\n"
|
||||||
|
post = "a\ne\n" # deleted b,c,d
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
baseline = [
|
||||||
|
_diag(line=0, message="err on a"),
|
||||||
|
_diag(line=1, message="err on b"), # → deleted
|
||||||
|
_diag(line=2, message="err on c"), # → deleted
|
||||||
|
_diag(line=4, message="err on e"),
|
||||||
|
]
|
||||||
|
out = shift_baseline(baseline, shift)
|
||||||
|
assert [d["message"] for d in out] == ["err on a", "err on e"]
|
||||||
|
assert out[0]["range"]["start"]["line"] == 0
|
||||||
|
assert out[1]["range"]["start"]["line"] == 1
|
||||||
|
|
||||||
|
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
# End-to-end: simulate the delta-filter pipeline
|
||||||
|
# ----------------------------------------------------------------------
|
||||||
|
|
||||||
|
def test_pipeline_filters_shifted_baseline_under_strict_key():
|
||||||
|
"""The exact scenario the bug fix is for: an edit deletes lines,
|
||||||
|
every diagnostic below shifts, and the delta filter (strict key
|
||||||
|
+ shifted baseline) correctly identifies them as pre-existing."""
|
||||||
|
pre = "line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n"
|
||||||
|
# Delete lines 2,3,4 — pre-existing errors at lines 7,8 should
|
||||||
|
# appear at lines 4,5 post-edit and be filtered out.
|
||||||
|
post = "line0\nline1\nline5\nline6\nline7\nline8\nline9\n"
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
|
||||||
|
baseline = [_diag(line=7, message="X"), _diag(line=8, message="Y")]
|
||||||
|
post_diags = [_diag(line=4, message="X"), _diag(line=5, message="Y")]
|
||||||
|
|
||||||
|
shifted_baseline = shift_baseline(baseline, shift)
|
||||||
|
seen = {_diag_key(d) for d in shifted_baseline}
|
||||||
|
new_diags = [d for d in post_diags if _diag_key(d) not in seen]
|
||||||
|
|
||||||
|
# Both errors were pre-existing — filtered out.
|
||||||
|
assert new_diags == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_pipeline_preserves_new_instance_at_different_line():
|
||||||
|
"""The case content-only keys would miss: the model introduces a
|
||||||
|
SECOND instance of the same error class at a new location. The
|
||||||
|
new instance must surface."""
|
||||||
|
pre = "good\ngood\ngood\n"
|
||||||
|
post = "good\nbad\ngood\nbad\n" # added 2 new error lines
|
||||||
|
shift = build_line_shift(pre, post)
|
||||||
|
|
||||||
|
baseline = [_diag(line=0, message="bad style")] # pre-existing
|
||||||
|
post_diags = [
|
||||||
|
_diag(line=0, message="bad style"), # pre-existing
|
||||||
|
_diag(line=1, message="bad style"), # NEW — different line
|
||||||
|
_diag(line=3, message="bad style"), # NEW — different line
|
||||||
|
]
|
||||||
|
|
||||||
|
shifted_baseline = shift_baseline(baseline, shift)
|
||||||
|
seen = {_diag_key(d) for d in shifted_baseline}
|
||||||
|
new_diags = [d for d in post_diags if _diag_key(d) not in seen]
|
||||||
|
|
||||||
|
# Two genuinely new instances must be surfaced.
|
||||||
|
assert len(new_diags) == 2
|
||||||
|
assert {d["range"]["start"]["line"] for d in new_diags} == {1, 3}
|
||||||
|
|
@ -130,6 +130,35 @@ def test_service_e2e_delta_filter(mock_pyright):
|
||||||
svc.shutdown()
|
svc.shutdown()
|
||||||
|
|
||||||
|
|
||||||
|
def test_service_e2e_delta_filter_with_line_shift(mock_pyright):
|
||||||
|
"""End-to-end: an edit that shifts the diagnostic's line still
|
||||||
|
filters correctly when ``line_shift`` is supplied.
|
||||||
|
|
||||||
|
The mock LSP server emits a fixed error at line 0; for this test
|
||||||
|
we don't need to actually shift the server's output — we just
|
||||||
|
need to prove that supplying a line_shift through the API works
|
||||||
|
and doesn't break the existing delta path. The unit tests in
|
||||||
|
test_delta_key.py cover the shift semantics in detail.
|
||||||
|
"""
|
||||||
|
repo = mock_pyright
|
||||||
|
f = repo / "x.py"
|
||||||
|
f.write_text("print('hi')\n")
|
||||||
|
|
||||||
|
svc = LSPService(
|
||||||
|
enabled=True,
|
||||||
|
wait_mode="document",
|
||||||
|
wait_timeout=3.0,
|
||||||
|
install_strategy="manual",
|
||||||
|
)
|
||||||
|
try:
|
||||||
|
svc.snapshot_baseline(str(f))
|
||||||
|
# Identity shift — should behave exactly like no shift.
|
||||||
|
new_diags = svc.get_diagnostics_sync(str(f), line_shift=lambda L: L)
|
||||||
|
assert new_diags == []
|
||||||
|
finally:
|
||||||
|
svc.shutdown()
|
||||||
|
|
||||||
|
|
||||||
def test_service_status_includes_clients(mock_pyright):
|
def test_service_status_includes_clients(mock_pyright):
|
||||||
repo = mock_pyright
|
repo = mock_pyright
|
||||||
f = repo / "x.py"
|
f = repo / "x.py"
|
||||||
|
|
|
||||||
|
|
@ -909,19 +909,29 @@ class ShellFileOperations(FileOperations):
|
||||||
if _is_write_denied(path):
|
if _is_write_denied(path):
|
||||||
return WriteResult(error=f"Write denied: '{path}' is a protected system/credential file.")
|
return WriteResult(error=f"Write denied: '{path}' is a protected system/credential file.")
|
||||||
|
|
||||||
# Capture pre-write content for lint-delta computation. Only do this
|
# Capture pre-write content. Two consumers want it:
|
||||||
# when an in-process OR shell linter exists for this extension — no
|
#
|
||||||
# point paying for the read otherwise. For in-process linters we
|
# 1. The lint-delta layer (for in-process linters like ast.parse
|
||||||
# pass the content directly; for shell linters the pre-state isn't
|
# and json.loads) needs the previous content to compute the
|
||||||
# useful (we'd have to re-write-read to lint the old version, which
|
# set of NEW lint errors introduced by this write.
|
||||||
# defeats the purpose), so we skip the capture and accept the naive
|
# 2. The LSP layer needs pre/post content to build a line-shift
|
||||||
# "all errors" report.
|
# map — pre-existing diagnostics below the edit point shift
|
||||||
|
# when lines are added/removed, and the shift map remaps
|
||||||
|
# baseline diagnostics into post-edit coordinates so the
|
||||||
|
# strict (range-aware) delta key matches.
|
||||||
|
#
|
||||||
|
# The set of extensions we capture pre_content for is therefore
|
||||||
|
# the UNION of in-process lint coverage and LSP coverage. For
|
||||||
|
# extensions outside both sets (binaries, opaque formats),
|
||||||
|
# skipping the read keeps the hot path fast.
|
||||||
ext = os.path.splitext(path)[1].lower()
|
ext = os.path.splitext(path)[1].lower()
|
||||||
pre_content: Optional[str] = None
|
pre_content: Optional[str] = None
|
||||||
if ext in LINTERS_INPROC:
|
want_pre = ext in LINTERS_INPROC or self._lsp_handles_extension(ext)
|
||||||
|
if want_pre:
|
||||||
# Best-effort read; failure (file missing, permission) leaves
|
# Best-effort read; failure (file missing, permission) leaves
|
||||||
# pre_content as None which makes the delta step degrade
|
# pre_content as None which makes both downstream consumers
|
||||||
# gracefully to "report all errors".
|
# degrade gracefully (lint reports all errors; LSP skips the
|
||||||
|
# shift map).
|
||||||
read_cmd = f"cat {self._escape_shell_arg(path)} 2>/dev/null"
|
read_cmd = f"cat {self._escape_shell_arg(path)} 2>/dev/null"
|
||||||
read_result = self._exec(read_cmd)
|
read_result = self._exec(read_cmd)
|
||||||
if read_result.exit_code == 0 and read_result.stdout:
|
if read_result.exit_code == 0 and read_result.stdout:
|
||||||
|
|
@ -966,11 +976,15 @@ class ShellFileOperations(FileOperations):
|
||||||
|
|
||||||
# Semantic diagnostics from the LSP layer — separate channel.
|
# Semantic diagnostics from the LSP layer — separate channel.
|
||||||
# Only fired when the syntax tier reported clean (no point asking
|
# Only fired when the syntax tier reported clean (no point asking
|
||||||
# an LSP for a file that won't even parse). Best-effort:
|
# an LSP for a file that won't even parse). Pass pre/post
|
||||||
# ``""`` is returned for any failure path.
|
# content so the LSP layer can build a line-shift map and
|
||||||
|
# remap baseline diagnostics into post-edit coordinates.
|
||||||
|
# Best-effort: ``""`` is returned for any failure path.
|
||||||
lsp_diagnostics: Optional[str] = None
|
lsp_diagnostics: Optional[str] = None
|
||||||
if lint_result.success or lint_result.skipped:
|
if lint_result.success or lint_result.skipped:
|
||||||
block = self._maybe_lsp_diagnostics(path)
|
block = self._maybe_lsp_diagnostics(
|
||||||
|
path, pre_content=pre_content, post_content=content
|
||||||
|
)
|
||||||
if block:
|
if block:
|
||||||
lsp_diagnostics = block
|
lsp_diagnostics = block
|
||||||
|
|
||||||
|
|
@ -1295,6 +1309,29 @@ class ShellFileOperations(FileOperations):
|
||||||
return False
|
return False
|
||||||
return isinstance(env, LocalEnvironment)
|
return isinstance(env, LocalEnvironment)
|
||||||
|
|
||||||
|
def _lsp_handles_extension(self, ext: str) -> bool:
|
||||||
|
"""Return True iff some registered LSP server claims this extension.
|
||||||
|
|
||||||
|
Used to decide whether to capture pre-write content for the
|
||||||
|
line-shift map. Capturing is cheap (one ``cat`` on the host)
|
||||||
|
but pointless if no LSP would ever look at the file.
|
||||||
|
|
||||||
|
Safe to call on remote backends — the registry is purely
|
||||||
|
in-process metadata; we still gate the actual LSP path on
|
||||||
|
:meth:`_lsp_local_only`.
|
||||||
|
"""
|
||||||
|
if not ext:
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
from agent.lsp.servers import SERVERS
|
||||||
|
except Exception: # noqa: BLE001
|
||||||
|
return False
|
||||||
|
ext_lower = ext.lower()
|
||||||
|
for srv in SERVERS:
|
||||||
|
if ext_lower in srv.extensions:
|
||||||
|
return True
|
||||||
|
return False
|
||||||
|
|
||||||
def _snapshot_lsp_baseline(self, path: str) -> None:
|
def _snapshot_lsp_baseline(self, path: str) -> None:
|
||||||
"""Capture pre-edit LSP diagnostics so the post-write delta is correct.
|
"""Capture pre-edit LSP diagnostics so the post-write delta is correct.
|
||||||
|
|
||||||
|
|
@ -1318,12 +1355,25 @@ class ShellFileOperations(FileOperations):
|
||||||
except Exception: # noqa: BLE001
|
except Exception: # noqa: BLE001
|
||||||
pass
|
pass
|
||||||
|
|
||||||
def _maybe_lsp_diagnostics(self, path: str) -> str:
|
def _maybe_lsp_diagnostics(
|
||||||
|
self,
|
||||||
|
path: str,
|
||||||
|
*,
|
||||||
|
pre_content: Optional[str] = None,
|
||||||
|
post_content: Optional[str] = None,
|
||||||
|
) -> str:
|
||||||
"""Best-effort LSP semantic diagnostics for ``path``.
|
"""Best-effort LSP semantic diagnostics for ``path``.
|
||||||
|
|
||||||
Returns a formatted ``<diagnostics>`` block, or empty string
|
Returns a formatted ``<diagnostics>`` block, or empty string
|
||||||
when LSP is unavailable / disabled / produced no errors.
|
when LSP is unavailable / disabled / produced no errors.
|
||||||
|
|
||||||
|
When both ``pre_content`` and ``post_content`` are provided,
|
||||||
|
a line-shift map is built and passed to the LSPService so
|
||||||
|
baseline diagnostics are remapped into post-edit coordinates
|
||||||
|
before the set-difference. Without this, edits that delete
|
||||||
|
or insert lines surface every pre-existing diagnostic below
|
||||||
|
the edit point as "introduced by this edit".
|
||||||
|
|
||||||
Wraps everything in a try/except so a misbehaving LSP server
|
Wraps everything in a try/except so a misbehaving LSP server
|
||||||
can't break a write. This intentionally swallows all errors
|
can't break a write. This intentionally swallows all errors
|
||||||
— the calling tier already returned a clean syntax result, so
|
— the calling tier already returned a clean syntax result, so
|
||||||
|
|
@ -1344,8 +1394,20 @@ class ShellFileOperations(FileOperations):
|
||||||
return ""
|
return ""
|
||||||
if svc is None or not svc.enabled_for(path):
|
if svc is None or not svc.enabled_for(path):
|
||||||
return ""
|
return ""
|
||||||
|
|
||||||
|
# Build a line-shift map when we have both pre and post — it
|
||||||
|
# remaps baseline diagnostics into post-edit coordinates so
|
||||||
|
# the strict (range-aware) delta key matches correctly.
|
||||||
|
line_shift = None
|
||||||
|
if pre_content is not None and post_content is not None and pre_content != post_content:
|
||||||
|
try:
|
||||||
|
from agent.lsp.range_shift import build_line_shift
|
||||||
|
line_shift = build_line_shift(pre_content, post_content)
|
||||||
|
except Exception: # noqa: BLE001
|
||||||
|
line_shift = None
|
||||||
|
|
||||||
try:
|
try:
|
||||||
diagnostics = svc.get_diagnostics_sync(path, delta=True)
|
diagnostics = svc.get_diagnostics_sync(path, delta=True, line_shift=line_shift)
|
||||||
except Exception: # noqa: BLE001
|
except Exception: # noqa: BLE001
|
||||||
return ""
|
return ""
|
||||||
if not diagnostics:
|
if not diagnostics:
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue