mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-07 08:02:23 +00:00
fix(lint): skip per-file shell linter when LSP will handle the file (#29054)
* fix(lint): skip per-file shell linter when LSP will handle the file `_check_lint` ran `npx tsc --noEmit FILE.ts` after every `.ts`/`.tsx` edit. `tsc` ignores `tsconfig.json` when given an explicit file argument (documented quirk) and defaults to no-lib / ES5, so every ES2015+ stdlib reference reports as missing: - `Cannot find global value 'Promise'` - `Cannot find name 'Map' / 'Set' / 'ReadonlySet' / 'Iterable'` - `Property 'isFinite' does not exist on type 'NumberConstructor'` - `Module 'phaser' can only be default-imported using esModuleInterop` - `import.meta is only allowed when --module is es2020+` On real TypeScript projects this floods the `lint` field on WriteResult / PatchResult with up to 25K tokens of false positives per edit. The delta filter in `_check_lint_delta` is supposed to mask them, but a tiny edit shifts line numbers and every phantom resurfaces as "introduced by this edit". The result is a 1MB+ phantom-error dump on every patch that eats the agent's context budget. Same shape for `.go` (`go vet` outside a module) and `.rs` (`rustfmt --check` outside a Cargo project). PR #24168 added an LSP tier on top of this — real `tsserver` / `gopls` / `rust-analyzer` diagnostics surface in the separate `lsp_diagnostics` field. But the broken shell linter kept running underneath, so the phantom-error dump kept happening even when LSP was giving us a clean authoritative signal. This change short-circuits the shell linter for the structurally-broken extensions (`.ts`, `.tsx`, `.go`, `.rs`) when an LSP server is active and claims the file via `LSPService.enabled_for(path)`. The LSP tier runs as before and carries the real diagnostics in `lsp_diagnostics`. Other shell linters (`py_compile`, `node --check`) keep running unconditionally — they're fast, file-local, and correct. Default behavior (LSP disabled, LSP misconfigured, remote backend, file outside a workspace) is unchanged — the existing fallback paths trigger when `_lsp_will_handle` returns False, so users who haven't opted into LSP get the same shell-linter behavior they had before. Drive-by: `.tsx` was missing from the `LINTERS` table entirely, so TS React files got no post-edit syntax check at all. Added it for symmetry; in practice it now hits the LSP-skip path. Tests: - `tests/agent/lsp/test_shell_linter_lsp_skip.py` — 14 tests covering: * skip happens for each redundant extension when LSP claims the file (asserted by patching `_exec` to raise on any shell-linter call) * shell linter still runs when LSP is inactive (regression guard) * `.py` / `.js` continue to run unconditionally even with LSP active * `_lsp_will_handle` is exception-safe: returns False on None service, remote backend, or `enabled_for` raising * `.tsx` is in both `LINTERS` and `_SHELL_LINTER_LSP_REDUNDANT` - All pre-existing tests in `tests/agent/lsp/` and `tests/tools/test_file_operations*.py` still pass (233/233). * fix(lint): address Copilot review on #29054 Two fixes from copilot-pull-request-reviewer on PR #29054: 1. `.tsx` regression with LSP disabled (https://github.com/NousResearch/hermes-agent/pull/29054#discussion_r3271017282) The first revision added `.tsx` to the `LINTERS` table so that TypeScript React files would hit the LSP skip path. Side effect: when LSP is *disabled* (the default), `.tsx` edits would suddenly run `npx tsc --noEmit FILE.tsx` and inherit the same phantom-error dump this PR is supposed to fix. Pre-PR behavior was implicit `skipped` (no `LINTERS` entry); restore that. - Remove `.tsx` from `LINTERS`. - Remove `.tsx` from `_SHELL_LINTER_LSP_REDUNDANT` (the skip path is unreachable without a `LINTERS` entry — falls through to `ext not in LINTERS` first). - When LSP IS enabled, `.tsx` is still covered by the LSP tier via `_maybe_lsp_diagnostics` (typescript-language-server's `extensions` tuple includes `.tsx`), so the diagnostics still surface — just on the `lsp_diagnostics` channel, not `lint`. - Update test_shell_linter_lsp_skip.py to reflect this contract (drop `.tsx` from the parametrize lists; add `test_tsx_stays_out_of_linters_table_for_default_compatibility` and `test_tsx_default_check_lint_returns_skipped`). 2. V4A patches dropped `WriteResult.lsp_diagnostics` (https://github.com/NousResearch/hermes-agent/pull/29054#discussion_r3271017295) `tools/patch_parser.py::apply_v4a_operations` calls `file_ops.write_file()` per operation, then calls `_check_lint()` directly afterwards — but never propagates `WriteResult.lsp_diagnostics` to the `PatchResult`. The shell-linter skip introduced in this PR makes the gap visible: a `.ts` / `.go` / `.rs` V4A patch with LSP active would return `lint = {f: {skipped: True}}` and zero diagnostics from any channel. - `_apply_add` and `_apply_update` now return `Tuple[bool, str, Optional[str]]` where the third element is `WriteResult.lsp_diagnostics` (or `None` on failure / no diags). - `_apply_delete` and `_apply_move` stay 2-tuples — they don't produce diagnostics, no write goes through `write_file`. - `apply_v4a_operations` accumulates per-file diagnostics blocks and surfaces a combined block on `PatchResult.lsp_diagnostics`. Each block already carries its `<diagnostics file="...">` header from `LSPService.report_for_file`, so concatenation preserves per-file attribution. Tests added (`test_patch_parser.py::TestV4ALspDiagnosticsPropagation`): - ADD op: `WriteResult.lsp_diagnostics` flows to `PatchResult` - UPDATE op: same - No diagnostics → `PatchResult.lsp_diagnostics is None` (not "") - Multi-file patch: combined block contains every per-file block Verification: - Targeted test scope: 257/257 pass (tests/agent/lsp/, tests/tools/test_file_operations*.py, tests/tools/test_patch_parser.py) - Wider sweep: 5400 pass; 11 failures all pre-existing on origin/main (file_staleness / file_read_guards / file_state_registry — unrelated macOS /var/folders tmp-path sensitivity issues, confirmed by re-running on a clean origin/main checkout) * docs(test): align shell-linter LSP skip docstring with .tsx behavior Copilot review feedback (review #4324947616, comment #3271049036): the test module docstring still listed .tsx alongside .ts/.go/.rs in the skip contract, but .tsx is now intentionally NOT in LINTERS or _SHELL_LINTER_LSP_REDUNDANT. Updated the bullet list to drop .tsx from the skip contract and added a paragraph documenting why .tsx is left out (preserves pre-PR implicit-skip behavior for LSP-disabled users; LSP coverage still happens via _maybe_lsp_diagnostics). * test(lsp): drop unused tmp_path from _make_fops helper Copilot review #3271069484: the helper accepted tmp_path but never used it. Callers still need tmp_path themselves for the file they're asserting against, so we just drop the helper's parameter.
This commit is contained in:
brooklyn!
GitHub
parent
6a6766fb89
commit
5e743559e0
4 changed files with 474 additions and 11 deletions
|
|
@ -326,6 +326,44 @@ LINTERS = {
|
|||
'.rs': 'rustfmt --check {file} 2>&1',
|
||||
}
|
||||
|
||||
# Extensions where the per-file shell linter is structurally weaker than
|
||||
# a real LSP server AND produces phantom errors on real-world projects:
|
||||
#
|
||||
# - ``.ts``: ``tsc --noEmit FILE.ts`` ignores ``tsconfig.json`` and
|
||||
# defaults to no-lib / ES5, so every ES2015+ stdlib reference
|
||||
# (``Promise``, ``Map``, ``Set``, ``ReadonlySet``, ``Iterable``,
|
||||
# ``Math.imul``, ``Number.isFinite``, etc.) reports as missing. This
|
||||
# floods the agent's lint field with 20K+ tokens of false positives on
|
||||
# every edit. No supported tsc flag fixes the single-file invocation;
|
||||
# the canonical replacement is ``tsserver`` via LSP, which respects
|
||||
# tsconfig and gives true diagnostics.
|
||||
#
|
||||
# ``.tsx`` is intentionally NOT in ``LINTERS`` (and therefore not
|
||||
# here): it has no shell linter entry, so it falls through to the
|
||||
# ``ext not in LINTERS`` skip case unchanged. Pre-PR behavior:
|
||||
# ``.tsx`` was implicitly ``skipped``. Keeping it that way means
|
||||
# ``.tsx`` edits with LSP disabled get no per-file syntax check
|
||||
# (same as before this PR) instead of the broken ``tsc`` invocation
|
||||
# that ``.ts`` used to get. When LSP is enabled, ``.tsx`` is covered
|
||||
# by the LSP tier via ``_maybe_lsp_diagnostics`` exactly as ``.ts``.
|
||||
#
|
||||
# - ``.go``: ``go vet FILE.go`` fails outside a module / GOPATH with
|
||||
# "cannot find package" — already partially handled by
|
||||
# ``_LINTER_UNUSABLE_PATTERNS`` but only when the package error is the
|
||||
# ONLY output; mixed real+phantom output still leaks through.
|
||||
# ``gopls`` is the canonical replacement.
|
||||
#
|
||||
# - ``.rs``: ``rustfmt --check FILE.rs`` is style, not type-checking, and
|
||||
# rejects non-Cargo project files. ``rust-analyzer`` is the canonical
|
||||
# replacement.
|
||||
#
|
||||
# When the LSP service is configured AND ``enabled_for(path)`` for this
|
||||
# extension's file, ``_check_lint`` skips the shell linter for these
|
||||
# extensions — the ``lsp_diagnostics`` channel carries the real signal.
|
||||
# Everything else in ``LINTERS`` (Python ``py_compile``, ``node --check``)
|
||||
# is fast, file-local, and correct, so it runs unconditionally.
|
||||
_SHELL_LINTER_LSP_REDUNDANT = frozenset({'.ts', '.go', '.rs'})
|
||||
|
||||
|
||||
# Patterns that indicate the linter base command exists on PATH but
|
||||
# couldn't actually run — e.g. ``npx tsc`` when tsc isn't installed in
|
||||
|
|
@ -1169,6 +1207,19 @@ class ShellFileOperations(FileOperations):
|
|||
if ext not in LINTERS:
|
||||
return LintResult(skipped=True, message=f"No linter for {ext} files")
|
||||
|
||||
# If a real LSP server is active and claims this file, skip the
|
||||
# shell linter for extensions whose per-file shell invocation is
|
||||
# structurally weaker / floods phantom errors. See
|
||||
# ``_SHELL_LINTER_LSP_REDUNDANT`` above for the rationale per ext.
|
||||
# The LSP tier runs separately via ``_maybe_lsp_diagnostics`` and
|
||||
# carries the real diagnostics in ``lsp_diagnostics`` on the
|
||||
# WriteResult / PatchResult.
|
||||
if ext in _SHELL_LINTER_LSP_REDUNDANT and self._lsp_will_handle(path):
|
||||
return LintResult(
|
||||
skipped=True,
|
||||
message=f"LSP server handles {ext} — shell linter skipped",
|
||||
)
|
||||
|
||||
linter_cmd = LINTERS[ext]
|
||||
# Extract the base command (first word)
|
||||
base_cmd = linter_cmd.split()[0]
|
||||
|
|
@ -1332,6 +1383,40 @@ class ShellFileOperations(FileOperations):
|
|||
return True
|
||||
return False
|
||||
|
||||
def _lsp_will_handle(self, path: str) -> bool:
|
||||
"""Return True iff the LSP service is active AND will lint this file.
|
||||
|
||||
Stronger than :meth:`_lsp_handles_extension` — that one only checks
|
||||
the static server registry. This one additionally requires the
|
||||
LSP service to be configured/enabled and the file to pass
|
||||
:meth:`agent.lsp.manager.LSPService.enabled_for` (which gates on
|
||||
workspace detection, disabled-server set, and the broken-pair
|
||||
short-circuit).
|
||||
|
||||
Used by :meth:`_check_lint` to decide whether to skip the per-file
|
||||
shell linter for extensions in ``_SHELL_LINTER_LSP_REDUNDANT``.
|
||||
|
||||
Best-effort: any failure path returns False so the shell linter
|
||||
runs as before — never suppress lint based on an LSP probe that
|
||||
couldn't actually answer the question.
|
||||
"""
|
||||
if not self._lsp_local_only():
|
||||
return False
|
||||
try:
|
||||
from agent.lsp import get_service
|
||||
except Exception: # noqa: BLE001
|
||||
return False
|
||||
try:
|
||||
svc = get_service()
|
||||
except Exception: # noqa: BLE001
|
||||
return False
|
||||
if svc is None:
|
||||
return False
|
||||
try:
|
||||
return bool(svc.enabled_for(path))
|
||||
except Exception: # noqa: BLE001
|
||||
return False
|
||||
|
||||
def _snapshot_lsp_baseline(self, path: str) -> None:
|
||||
"""Capture pre-edit LSP diagnostics so the post-write delta is correct.
|
||||
|
||||
|
|
|
|||
|
|
@ -363,6 +363,12 @@ def apply_v4a_operations(operations: List[PatchOperation],
|
|||
files_created = []
|
||||
files_deleted = []
|
||||
all_diffs = []
|
||||
# Per-file LSP diagnostics blocks captured from underlying write_file
|
||||
# calls. V4A bypasses the WriteResult / PatchResult plumbing that
|
||||
# write_file and patch_replace use, so without explicit propagation
|
||||
# the LSP tier's output gets silently dropped — see
|
||||
# ``PatchResult.lsp_diagnostics`` aggregation below.
|
||||
lsp_blocks: List[str] = []
|
||||
errors = []
|
||||
|
||||
for op in operations:
|
||||
|
|
@ -372,6 +378,8 @@ def apply_v4a_operations(operations: List[PatchOperation],
|
|||
if result[0]:
|
||||
files_created.append(op.file_path)
|
||||
all_diffs.append(result[1])
|
||||
if result[2]:
|
||||
lsp_blocks.append(result[2])
|
||||
else:
|
||||
errors.append(f"Failed to add {op.file_path}: {result[1]}")
|
||||
|
||||
|
|
@ -396,6 +404,8 @@ def apply_v4a_operations(operations: List[PatchOperation],
|
|||
if result[0]:
|
||||
files_modified.append(op.file_path)
|
||||
all_diffs.append(result[1])
|
||||
if result[2]:
|
||||
lsp_blocks.append(result[2])
|
||||
else:
|
||||
errors.append(f"Failed to update {op.file_path}: {result[1]}")
|
||||
|
||||
|
|
@ -411,6 +421,13 @@ def apply_v4a_operations(operations: List[PatchOperation],
|
|||
|
||||
combined_diff = '\n'.join(all_diffs)
|
||||
|
||||
# Combine per-file LSP diagnostics blocks. Each block already has
|
||||
# the ``<diagnostics file="...">`` header from
|
||||
# ``LSPService.report_for_file`` so concatenation is safe — the
|
||||
# agent (and any downstream parsers) can still attribute each
|
||||
# diagnostic to its file.
|
||||
combined_lsp = "\n\n".join(lsp_blocks) if lsp_blocks else None
|
||||
|
||||
if errors:
|
||||
return PatchResult(
|
||||
success=False,
|
||||
|
|
@ -419,6 +436,7 @@ def apply_v4a_operations(operations: List[PatchOperation],
|
|||
files_created=files_created,
|
||||
files_deleted=files_deleted,
|
||||
lint=lint_results if lint_results else None,
|
||||
lsp_diagnostics=combined_lsp,
|
||||
error="Apply phase failed (state may be inconsistent — run `git diff` to assess):\n"
|
||||
+ "\n".join(f" • {e}" for e in errors),
|
||||
)
|
||||
|
|
@ -430,11 +448,19 @@ def apply_v4a_operations(operations: List[PatchOperation],
|
|||
files_created=files_created,
|
||||
files_deleted=files_deleted,
|
||||
lint=lint_results if lint_results else None,
|
||||
lsp_diagnostics=combined_lsp,
|
||||
)
|
||||
|
||||
|
||||
def _apply_add(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
||||
"""Apply an add file operation."""
|
||||
def _apply_add(op: PatchOperation, file_ops: Any) -> Tuple[bool, str, Optional[str]]:
|
||||
"""Apply an add file operation.
|
||||
|
||||
Returns ``(success, diff_or_error, lsp_diagnostics)``. The third
|
||||
element carries the formatted ``<diagnostics>`` block from
|
||||
:class:`WriteResult.lsp_diagnostics` so V4A patches can surface
|
||||
semantic diagnostics from the LSP layer — without this, the LSP
|
||||
tier would silently swallow them on the V4A code path.
|
||||
"""
|
||||
# Extract content from hunks (all + lines)
|
||||
content_lines = []
|
||||
for hunk in op.hunks:
|
||||
|
|
@ -446,12 +472,12 @@ def _apply_add(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
|||
|
||||
result = file_ops.write_file(op.file_path, content)
|
||||
if result.error:
|
||||
return False, result.error
|
||||
return False, result.error, None
|
||||
|
||||
diff = f"--- /dev/null\n+++ b/{op.file_path}\n"
|
||||
diff += '\n'.join(f"+{line}" for line in content_lines)
|
||||
|
||||
return True, diff
|
||||
return True, diff, getattr(result, "lsp_diagnostics", None)
|
||||
|
||||
|
||||
def _apply_delete(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
||||
|
|
@ -485,8 +511,12 @@ def _apply_move(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
|||
return True, diff
|
||||
|
||||
|
||||
def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
||||
"""Apply an update file operation."""
|
||||
def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str, Optional[str]]:
|
||||
"""Apply an update file operation.
|
||||
|
||||
Returns ``(success, diff_or_error, lsp_diagnostics)`` — see
|
||||
:func:`_apply_add` for the rationale on the third element.
|
||||
"""
|
||||
# Deferred import: breaks the patch_parser ↔ fuzzy_match circular dependency
|
||||
from tools.fuzzy_match import fuzzy_find_and_replace
|
||||
|
||||
|
|
@ -494,7 +524,7 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
|||
read_result = file_ops.read_file_raw(op.file_path)
|
||||
|
||||
if read_result.error:
|
||||
return False, f"Cannot read file: {read_result.error}"
|
||||
return False, f"Cannot read file: {read_result.error}", None
|
||||
|
||||
current_content = read_result.content
|
||||
|
||||
|
|
@ -549,7 +579,7 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
|||
err_msg += format_no_match_hint(error, 0, search_pattern, new_content)
|
||||
except Exception:
|
||||
pass
|
||||
return False, err_msg
|
||||
return False, err_msg, None
|
||||
else:
|
||||
# Addition-only hunk (no context or removed lines).
|
||||
# Insert at the location indicated by the context hint, or at end of file.
|
||||
|
|
@ -563,7 +593,7 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
|||
return False, (
|
||||
f"Addition-only hunk: context hint '{hunk.context_hint}' is ambiguous "
|
||||
f"({occurrences} occurrences) — provide a more unique hint"
|
||||
)
|
||||
), None
|
||||
else:
|
||||
hint_pos = new_content.find(hunk.context_hint)
|
||||
# Insert after the line containing the context hint
|
||||
|
|
@ -578,7 +608,7 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
|||
# Write new content
|
||||
write_result = file_ops.write_file(op.file_path, new_content)
|
||||
if write_result.error:
|
||||
return False, write_result.error
|
||||
return False, write_result.error, None
|
||||
|
||||
# Generate diff
|
||||
diff_lines = difflib.unified_diff(
|
||||
|
|
@ -589,4 +619,4 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
|||
)
|
||||
diff = ''.join(diff_lines)
|
||||
|
||||
return True, diff
|
||||
return True, diff, getattr(write_result, "lsp_diagnostics", None)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue