From 5e743559e0157df42e0f640cd06d736e898370d0 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Wed, 20 May 2026 01:46:40 -0500 Subject: [PATCH] fix(lint): skip per-file shell linter when LSP will handle the file (#29054) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 `` 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. --- tests/agent/lsp/test_shell_linter_lsp_skip.py | 210 ++++++++++++++++++ tests/tools/test_patch_parser.py | 138 ++++++++++++ tools/file_operations.py | 85 +++++++ tools/patch_parser.py | 52 ++++- 4 files changed, 474 insertions(+), 11 deletions(-) create mode 100644 tests/agent/lsp/test_shell_linter_lsp_skip.py diff --git a/tests/agent/lsp/test_shell_linter_lsp_skip.py b/tests/agent/lsp/test_shell_linter_lsp_skip.py new file mode 100644 index 00000000000..a101fa9e1be --- /dev/null +++ b/tests/agent/lsp/test_shell_linter_lsp_skip.py @@ -0,0 +1,210 @@ +"""Skip the per-file shell linter when LSP will handle the same file. + +The per-file ``npx tsc --noEmit FILE.ts`` shell linter cannot see +``tsconfig.json`` (a documented ``tsc`` quirk: explicit file args bypass +the project config), so it defaults to no-lib / ES5 and floods the +agent's lint field with phantom "Cannot find 'Promise' / 'Map' / 'Set' / +'ReadonlySet' / 'Iterable' / 'imul' / …" errors on every edit — up to +25K tokens per patch. The LSP tier (``tsserver`` via +typescript-language-server) reads tsconfig correctly and surfaces real +diagnostics in the ``lsp_diagnostics`` field of the WriteResult / +PatchResult. + +These tests pin the contract: + + - When LSP is active AND ``enabled_for(path)`` for a ``.ts`` / ``.go`` + / ``.rs`` file, ``_check_lint`` returns ``skipped`` without invoking + the shell linter at all. + - When LSP is inactive or disabled-for-path, the shell linter runs + exactly as before (regression guard for the default config). + - The skip only applies to extensions in + ``_SHELL_LINTER_LSP_REDUNDANT`` — Python ``py_compile`` and + ``node --check`` keep running unconditionally because they're fast, + file-local, and correct. + - ``.tsx`` is intentionally NOT in either ``LINTERS`` or + ``_SHELL_LINTER_LSP_REDUNDANT``: it had no ``LINTERS`` entry + pre-PR (so it was already implicitly ``skipped`` via the + ``ext not in LINTERS`` branch) and adding one would have inherited + ``.ts``'s broken ``tsc --noEmit FILE`` invocation for LSP-disabled + users. When LSP IS enabled, ``.tsx`` is still covered by + typescript-language-server via ``_maybe_lsp_diagnostics`` — the + diagnostics show up on ``lsp_diagnostics``, not ``lint``. +""" +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + + +def _make_fops(): + from tools.environments.local import LocalEnvironment + from tools.file_operations import ShellFileOperations + return ShellFileOperations(LocalEnvironment()) + + +@pytest.mark.parametrize("ext", [".ts", ".go", ".rs"]) +def test_shell_linter_skipped_when_lsp_will_handle(ext, tmp_path): + """When LSP is active and enabled_for(path), shell linter is skipped. + + The shell linter's _exec must NOT be called — that's the whole + point. We assert by patching ``_exec`` to raise, so any accidental + invocation surfaces as a test failure. + """ + fops = _make_fops() + src = tmp_path / f"bad{ext}" + src.write_text("intentionally invalid content\n") + + def _exec_must_not_run(*args, **kwargs): # pragma: no cover + raise AssertionError( + "shell linter was invoked despite LSP claiming the file" + ) + + with patch.object(fops, "_lsp_will_handle", return_value=True), \ + patch.object(fops, "_exec", side_effect=_exec_must_not_run), \ + patch.object(fops, "_has_command", return_value=True): + result = fops._check_lint(str(src)) + + assert result.skipped is True + assert "LSP" in (result.message or "") + + +@pytest.mark.parametrize("ext", [".ts", ".go", ".rs"]) +def test_shell_linter_runs_when_lsp_inactive(ext, tmp_path): + """When LSP is inactive (default config, no service, remote backend, ...), + the shell linter runs as before — no behavior change.""" + fops = _make_fops() + src = tmp_path / f"clean{ext}" + src.write_text("// content\n") + + fake_result = MagicMock() + fake_result.exit_code = 0 + fake_result.stdout = "" + + with patch.object(fops, "_lsp_will_handle", return_value=False), \ + patch.object(fops, "_exec", return_value=fake_result) as exec_mock, \ + patch.object(fops, "_has_command", return_value=True): + result = fops._check_lint(str(src)) + + # _exec must have been called — proving the shell linter ran. + assert exec_mock.called, "shell linter did NOT run when LSP was inactive" + assert result.success is True + + +@pytest.mark.parametrize("ext", [".py", ".js"]) +def test_lsp_does_not_skip_non_redundant_extensions(ext, tmp_path): + """``py_compile`` and ``node --check`` keep running even when an LSP + server (pyright/pylsp/typescript-language-server-for-JS) is active — + they're fast, file-local, and correct, so there's no upside to + suppressing them. + """ + fops = _make_fops() + src = tmp_path / f"clean{ext}" + src.write_text("# valid\n" if ext == ".py" else "// valid\n") + + fake_result = MagicMock() + fake_result.exit_code = 0 + fake_result.stdout = "" + + # Even with LSP claiming the file, the shell linter must still run + # for these extensions. + with patch.object(fops, "_lsp_will_handle", return_value=True), \ + patch.object(fops, "_exec", return_value=fake_result) as exec_mock, \ + patch.object(fops, "_has_command", return_value=True): + fops._check_lint(str(src)) + + assert exec_mock.called, ( + f"shell linter for {ext} did not run despite being in the " + "'always-run' set (py_compile / node --check)" + ) + + +def test_lsp_will_handle_returns_false_when_service_is_none(tmp_path): + """``_lsp_will_handle`` must return False when the LSP service hasn't + been initialized — otherwise we'd accidentally skip the shell linter + on systems where LSP isn't configured at all.""" + fops = _make_fops() + src = tmp_path / "foo.ts" + src.write_text("const x = 1\n") + + with patch.object(fops, "_lsp_local_only", return_value=True), \ + patch("agent.lsp.get_service", return_value=None): + assert fops._lsp_will_handle(str(src)) is False + + +def test_lsp_will_handle_returns_false_on_remote_backend(tmp_path): + """LSP servers run on the host process — remote backends (Docker, + SSH, Modal, …) keep files inside the sandbox where the host LSP + can't reach them. ``_lsp_will_handle`` must short-circuit before + calling into the service in that case.""" + fops = _make_fops() + src = tmp_path / "foo.ts" + src.write_text("const x = 1\n") + + with patch.object(fops, "_lsp_local_only", return_value=False), \ + patch("agent.lsp.get_service") as get_service_mock: + result = fops._lsp_will_handle(str(src)) + + assert result is False + # Importantly: we never even consulted the service. + assert not get_service_mock.called + + +def test_lsp_will_handle_swallows_enabled_for_exception(tmp_path): + """A flaky LSP service must never break the shell-linter fallback — + if ``enabled_for`` raises, we treat the file as "not handled" so the + shell linter still runs.""" + fops = _make_fops() + src = tmp_path / "foo.ts" + src.write_text("const x = 1\n") + + fake_svc = MagicMock() + fake_svc.enabled_for.side_effect = RuntimeError("server crashed") + + with patch.object(fops, "_lsp_local_only", return_value=True), \ + patch("agent.lsp.get_service", return_value=fake_svc): + assert fops._lsp_will_handle(str(src)) is False + + +def test_tsx_stays_out_of_linters_table_for_default_compatibility(): + """Regression: keep ``.tsx`` out of ``LINTERS`` so users with LSP + DISABLED don't suddenly get the broken ``npx tsc --noEmit FILE.tsx`` + invocation that ``.ts`` historically used to get. + + Pre-PR behavior: ``.tsx`` had no entry in ``LINTERS``, so it fell + through to ``ext not in LINTERS`` → ``LintResult(skipped=True, + message="No linter for .tsx files")``. This PR preserves that for + the default config. + + When LSP IS enabled, ``.tsx`` is still covered by the LSP tier via + ``_maybe_lsp_diagnostics`` (typescript-language-server claims + ``.tsx`` in its extensions list) — the diagnostics show up in the + ``lsp_diagnostics`` field, not the ``lint`` field. + """ + from tools.file_operations import LINTERS, _SHELL_LINTER_LSP_REDUNDANT + + assert ".tsx" not in LINTERS + assert ".tsx" not in _SHELL_LINTER_LSP_REDUNDANT + + +def test_tsx_default_check_lint_returns_skipped(tmp_path): + """End-to-end: ``.tsx`` files get ``LintResult(skipped=True)`` from + ``_check_lint`` regardless of LSP status — this is the no-regression + contract that addresses Copilot review #3271017282.""" + fops = _make_fops() + src = tmp_path / "foo.tsx" + src.write_text("export const X = () =>
\n") + + # Even with LSP claiming the file, no shell linter runs for .tsx + # because there's no LINTERS entry — the ``ext not in LINTERS`` + # branch fires before the LSP short-circuit is consulted. + with patch.object(fops, "_lsp_will_handle", return_value=True), \ + patch.object(fops, "_exec") as exec_mock: + result = fops._check_lint(str(src)) + + assert result.skipped is True + assert not exec_mock.called, "no shell linter should run for .tsx" + + +if __name__ == "__main__": # pragma: no cover + pytest.main([__file__, "-v"]) diff --git a/tests/tools/test_patch_parser.py b/tests/tools/test_patch_parser.py index 8c4a0c80a37..79077a84a16 100644 --- a/tests/tools/test_patch_parser.py +++ b/tests/tools/test_patch_parser.py @@ -509,3 +509,141 @@ class TestParseErrorSignalling: ops, err = parse_v4a_patch(patch) assert err is None assert len(ops) == 1 + + +class TestV4ALspDiagnosticsPropagation: + """V4A patches must surface ``WriteResult.lsp_diagnostics`` from the + underlying ``write_file`` calls on ``PatchResult.lsp_diagnostics``. + + Without explicit propagation the LSP tier's output gets silently + dropped on the V4A code path — see Copilot review #3271017295 on + PR #29054. The shell-linter LSP skip introduced by that PR makes + this gap visible: a ``.ts`` / ``.go`` / ``.rs`` V4A patch with LSP + active would otherwise return ``lint = {f: {skipped: True, ...}}`` + and zero diagnostics from any channel. + """ + + def _build_ops_writing(self, path: str, content: str): + """Build a single ADD operation that writes ``content`` to ``path``.""" + # Use the V4A parser so we don't have to construct PatchOperation + # / Hunk / Line objects by hand. + lines = "\n".join(f"+{line}" for line in content.splitlines()) + patch_text = ( + "*** Begin Patch\n" + f"*** Add File: {path}\n" + f"{lines}\n" + "*** End Patch" + ) + ops, err = parse_v4a_patch(patch_text) + assert err is None, err + return ops + + def test_lsp_diagnostics_propagated_from_write_file_on_add(self): + """ADD op: ``WriteResult.lsp_diagnostics`` flows through to + ``PatchResult.lsp_diagnostics``.""" + ops = self._build_ops_writing("foo.ts", "const x: number = 1\n") + + diag_block = ( + "\n" + "ERROR [1:7] some diagnostic\n" + "" + ) + + class FakeFileOps: + def write_file(self, path, content): + return SimpleNamespace(error=None, lsp_diagnostics=diag_block) + + def _check_lint(self, path): + return SimpleNamespace(to_dict=lambda: {"skipped": True}) + + result = apply_v4a_operations(ops, FakeFileOps()) + + assert result.success is True + assert result.lsp_diagnostics == diag_block + + def test_lsp_diagnostics_propagated_from_write_file_on_update(self): + """UPDATE op: ``WriteResult.lsp_diagnostics`` flows through to + ``PatchResult.lsp_diagnostics``.""" + patch_text = ( + "*** Begin Patch\n" + "*** Update File: bar.ts\n" + "-old\n" + "+new\n" + "*** End Patch" + ) + ops, err = parse_v4a_patch(patch_text) + assert err is None + + diag_block = ( + "\n" + "ERROR [3:1] something\n" + "" + ) + + class FakeFileOps: + def read_file_raw(self, path): + return SimpleNamespace(content="ctx\nold\nctx\n", error=None) + + def write_file(self, path, content): + return SimpleNamespace(error=None, lsp_diagnostics=diag_block) + + def _check_lint(self, path): + return SimpleNamespace(to_dict=lambda: {"skipped": True}) + + result = apply_v4a_operations(ops, FakeFileOps()) + + assert result.success is True + assert result.lsp_diagnostics == diag_block + + def test_lsp_diagnostics_none_when_no_blocks_emitted(self): + """When no underlying ``write_file`` produced diagnostics, the + aggregated field stays ``None`` (so it doesn't get serialized + as an empty string in ``PatchResult.to_dict``).""" + ops = self._build_ops_writing("foo.py", "x = 1\n") + + class FakeFileOps: + def write_file(self, path, content): + # lsp_diagnostics omitted entirely (older WriteResult shape). + return SimpleNamespace(error=None) + + def _check_lint(self, path): + return SimpleNamespace(to_dict=lambda: {"success": True}) + + result = apply_v4a_operations(ops, FakeFileOps()) + + assert result.success is True + assert result.lsp_diagnostics is None + + def test_lsp_diagnostics_combined_across_multiple_files(self): + """When several files in one V4A patch produce diagnostics, + each block appears in the combined output so per-file attribution + is preserved.""" + patch_text = ( + "*** Begin Patch\n" + "*** Add File: a.ts\n" + "+const a = 1\n" + "*** Add File: b.ts\n" + "+const b = 2\n" + "*** End Patch" + ) + ops, err = parse_v4a_patch(patch_text) + assert err is None + + per_file = { + "a.ts": "\nERR a\n", + "b.ts": "\nERR b\n", + } + + class FakeFileOps: + def write_file(self, path, content): + return SimpleNamespace(error=None, lsp_diagnostics=per_file[path]) + + def _check_lint(self, path): + return SimpleNamespace(to_dict=lambda: {"skipped": True}) + + result = apply_v4a_operations(ops, FakeFileOps()) + + assert result.success is True + assert result.lsp_diagnostics is not None + assert per_file["a.ts"] in result.lsp_diagnostics + assert per_file["b.ts"] in result.lsp_diagnostics diff --git a/tools/file_operations.py b/tools/file_operations.py index 13d9314b912..c25dc332cb0 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -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. diff --git a/tools/patch_parser.py b/tools/patch_parser.py index dacc6e855c3..e16cb446ee0 100644 --- a/tools/patch_parser.py +++ b/tools/patch_parser.py @@ -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 ```` 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 ```` 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)