mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-21 03:39:54 +00:00
5e743559e0
* 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.
650 lines
20 KiB
Python
650 lines
20 KiB
Python
"""Tests for the V4A patch format parser."""
|
|
|
|
from types import SimpleNamespace
|
|
|
|
from tools.patch_parser import (
|
|
OperationType,
|
|
apply_v4a_operations,
|
|
parse_v4a_patch,
|
|
)
|
|
|
|
|
|
class TestParseUpdateFile:
|
|
def test_basic_update(self):
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: src/main.py
|
|
@@ def greet @@
|
|
def greet():
|
|
- print("hello")
|
|
+ print("hi")
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 1
|
|
|
|
op = ops[0]
|
|
assert op.operation == OperationType.UPDATE
|
|
assert op.file_path == "src/main.py"
|
|
assert len(op.hunks) == 1
|
|
|
|
hunk = op.hunks[0]
|
|
assert hunk.context_hint == "def greet"
|
|
prefixes = [l.prefix for l in hunk.lines]
|
|
assert " " in prefixes
|
|
assert "-" in prefixes
|
|
assert "+" in prefixes
|
|
|
|
def test_multiple_hunks(self):
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: f.py
|
|
@@ first @@
|
|
a
|
|
-b
|
|
+c
|
|
@@ second @@
|
|
x
|
|
-y
|
|
+z
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 1
|
|
assert len(ops[0].hunks) == 2
|
|
assert ops[0].hunks[0].context_hint == "first"
|
|
assert ops[0].hunks[1].context_hint == "second"
|
|
|
|
|
|
class TestParseAddFile:
|
|
def test_add_file(self):
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Add File: new/module.py
|
|
+import os
|
|
+
|
|
+print("hello")
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 1
|
|
|
|
op = ops[0]
|
|
assert op.operation == OperationType.ADD
|
|
assert op.file_path == "new/module.py"
|
|
assert len(op.hunks) == 1
|
|
|
|
contents = [l.content for l in op.hunks[0].lines if l.prefix == "+"]
|
|
assert contents[0] == "import os"
|
|
assert contents[2] == 'print("hello")'
|
|
|
|
|
|
class TestParseDeleteFile:
|
|
def test_delete_file(self):
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Delete File: old/stuff.py
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 1
|
|
assert ops[0].operation == OperationType.DELETE
|
|
assert ops[0].file_path == "old/stuff.py"
|
|
|
|
|
|
class TestParseMoveFile:
|
|
def test_move_file(self):
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Move File: old/path.py -> new/path.py
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 1
|
|
assert ops[0].operation == OperationType.MOVE
|
|
assert ops[0].file_path == "old/path.py"
|
|
assert ops[0].new_path == "new/path.py"
|
|
|
|
|
|
class TestParseInvalidPatch:
|
|
def test_empty_patch_returns_empty_ops(self):
|
|
ops, err = parse_v4a_patch("")
|
|
assert err is None
|
|
assert ops == []
|
|
|
|
def test_no_begin_marker_still_parses(self):
|
|
patch = """\
|
|
*** Update File: f.py
|
|
line1
|
|
-old
|
|
+new
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 1
|
|
|
|
def test_multiple_operations(self):
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Add File: a.py
|
|
+content_a
|
|
*** Delete File: b.py
|
|
*** Update File: c.py
|
|
keep
|
|
-remove
|
|
+add
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 3
|
|
assert ops[0].operation == OperationType.ADD
|
|
assert ops[1].operation == OperationType.DELETE
|
|
assert ops[2].operation == OperationType.UPDATE
|
|
|
|
|
|
class TestApplyUpdate:
|
|
def test_preserves_non_prefix_pipe_characters_in_unmodified_lines(self):
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: sample.py
|
|
@@ result @@
|
|
result = 1
|
|
- return result
|
|
+ return result + 1
|
|
*** End Patch"""
|
|
operations, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
class FakeFileOps:
|
|
def __init__(self):
|
|
self.written = None
|
|
|
|
def read_file_raw(self, path):
|
|
return SimpleNamespace(
|
|
content=(
|
|
'def run():\n'
|
|
' cmd = "echo a | sed s/a/b/"\n'
|
|
' result = 1\n'
|
|
' return result'
|
|
),
|
|
error=None,
|
|
)
|
|
|
|
def write_file(self, path, content):
|
|
self.written = content
|
|
return SimpleNamespace(error=None)
|
|
|
|
file_ops = FakeFileOps()
|
|
|
|
result = apply_v4a_operations(operations, file_ops)
|
|
|
|
assert result.success is True
|
|
assert file_ops.written == (
|
|
'def run():\n'
|
|
' cmd = "echo a | sed s/a/b/"\n'
|
|
' result = 1\n'
|
|
' return result + 1'
|
|
)
|
|
|
|
|
|
class TestAdditionOnlyHunks:
|
|
"""Regression tests for #3081 — addition-only hunks were silently dropped."""
|
|
|
|
def test_addition_only_hunk_with_context_hint(self):
|
|
"""A hunk with only + lines should insert at the context hint location."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: src/app.py
|
|
@@ def main @@
|
|
+def helper():
|
|
+ return 42
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
assert len(ops) == 1
|
|
assert len(ops[0].hunks) == 1
|
|
|
|
hunk = ops[0].hunks[0]
|
|
# All lines should be additions
|
|
assert all(l.prefix == '+' for l in hunk.lines)
|
|
|
|
# Apply to a file that contains the context hint
|
|
class FakeFileOps:
|
|
written = None
|
|
def read_file_raw(self, path):
|
|
return SimpleNamespace(
|
|
content="def main():\n pass\n",
|
|
error=None,
|
|
)
|
|
def write_file(self, path, content):
|
|
self.written = content
|
|
return SimpleNamespace(error=None)
|
|
|
|
file_ops = FakeFileOps()
|
|
result = apply_v4a_operations(ops, file_ops)
|
|
assert result.success is True
|
|
assert "def helper():" in file_ops.written
|
|
assert "return 42" in file_ops.written
|
|
|
|
def test_addition_only_hunk_without_context_hint(self):
|
|
"""A hunk with only + lines and no context hint appends at end of file."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: src/app.py
|
|
+def new_func():
|
|
+ return True
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
class FakeFileOps:
|
|
written = None
|
|
def read_file_raw(self, path):
|
|
return SimpleNamespace(
|
|
content="existing = True\n",
|
|
error=None,
|
|
)
|
|
def write_file(self, path, content):
|
|
self.written = content
|
|
return SimpleNamespace(error=None)
|
|
|
|
file_ops = FakeFileOps()
|
|
result = apply_v4a_operations(ops, file_ops)
|
|
assert result.success is True
|
|
assert file_ops.written.endswith("def new_func():\n return True\n")
|
|
assert "existing = True" in file_ops.written
|
|
|
|
|
|
class TestReadFileRaw:
|
|
"""Bug 1 regression tests — files > 2000 lines and lines > 2000 chars."""
|
|
|
|
def test_apply_update_file_over_2000_lines(self):
|
|
"""A hunk targeting line 2200 must not truncate the file to 2000 lines."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: big.py
|
|
@@ marker_at_2200 @@
|
|
line_2200
|
|
-old_value
|
|
+new_value
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
# Build a 2500-line file; the hunk targets a region at line 2200
|
|
lines = [f"line_{i}" for i in range(1, 2501)]
|
|
lines[2199] = "line_2200" # index 2199 = line 2200
|
|
lines[2200] = "old_value"
|
|
file_content = "\n".join(lines)
|
|
|
|
class FakeFileOps:
|
|
written = None
|
|
def read_file_raw(self, path):
|
|
return SimpleNamespace(content=file_content, error=None)
|
|
def write_file(self, path, content):
|
|
self.written = content
|
|
return SimpleNamespace(error=None)
|
|
|
|
file_ops = FakeFileOps()
|
|
result = apply_v4a_operations(ops, file_ops)
|
|
assert result.success is True
|
|
written_lines = file_ops.written.split("\n")
|
|
assert len(written_lines) == 2500, (
|
|
f"Expected 2500 lines, got {len(written_lines)}"
|
|
)
|
|
assert "new_value" in file_ops.written
|
|
assert "old_value" not in file_ops.written
|
|
|
|
def test_apply_update_preserves_long_lines(self):
|
|
"""A line > 2000 chars must be preserved verbatim after an unrelated hunk."""
|
|
long_line = "x" * 3000
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: wide.py
|
|
@@ short_func @@
|
|
def short_func():
|
|
- return 1
|
|
+ return 2
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
file_content = f"def short_func():\n return 1\n{long_line}\n"
|
|
|
|
class FakeFileOps:
|
|
written = None
|
|
def read_file_raw(self, path):
|
|
return SimpleNamespace(content=file_content, error=None)
|
|
def write_file(self, path, content):
|
|
self.written = content
|
|
return SimpleNamespace(error=None)
|
|
|
|
file_ops = FakeFileOps()
|
|
result = apply_v4a_operations(ops, file_ops)
|
|
assert result.success is True
|
|
assert long_line in file_ops.written, "Long line was truncated"
|
|
assert "... [truncated]" not in file_ops.written
|
|
|
|
|
|
class TestValidationPhase:
|
|
"""Bug 2 regression tests — validation prevents partial apply."""
|
|
|
|
def test_validation_failure_writes_nothing(self):
|
|
"""If one hunk is invalid, no files should be written."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: a.py
|
|
def good():
|
|
- return 1
|
|
+ return 2
|
|
*** Update File: b.py
|
|
THIS LINE DOES NOT EXIST
|
|
- old
|
|
+ new
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
written = {}
|
|
|
|
class FakeFileOps:
|
|
def read_file_raw(self, path):
|
|
files = {
|
|
"a.py": "def good():\n return 1\n",
|
|
"b.py": "completely different content\n",
|
|
}
|
|
content = files.get(path)
|
|
if content is None:
|
|
return SimpleNamespace(content=None, error=f"File not found: {path}")
|
|
return SimpleNamespace(content=content, error=None)
|
|
|
|
def write_file(self, path, content):
|
|
written[path] = content
|
|
return SimpleNamespace(error=None)
|
|
|
|
result = apply_v4a_operations(ops, FakeFileOps())
|
|
assert result.success is False
|
|
assert written == {}, f"No files should have been written, got: {list(written.keys())}"
|
|
assert "validation failed" in result.error.lower()
|
|
|
|
def test_all_valid_operations_applied(self):
|
|
"""When all operations are valid, all files are written."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: a.py
|
|
def foo():
|
|
- return 1
|
|
+ return 2
|
|
*** Update File: b.py
|
|
def bar():
|
|
- pass
|
|
+ return True
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
written = {}
|
|
|
|
class FakeFileOps:
|
|
def read_file_raw(self, path):
|
|
files = {
|
|
"a.py": "def foo():\n return 1\n",
|
|
"b.py": "def bar():\n pass\n",
|
|
}
|
|
return SimpleNamespace(content=files[path], error=None)
|
|
|
|
def write_file(self, path, content):
|
|
written[path] = content
|
|
return SimpleNamespace(error=None)
|
|
|
|
result = apply_v4a_operations(ops, FakeFileOps())
|
|
assert result.success is True
|
|
assert set(written.keys()) == {"a.py", "b.py"}
|
|
|
|
|
|
class TestApplyDelete:
|
|
"""Tests for _apply_delete producing a real unified diff."""
|
|
|
|
def test_delete_diff_contains_removed_lines(self):
|
|
"""_apply_delete must embed the actual file content in the diff, not a placeholder."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Delete File: old/stuff.py
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
class FakeFileOps:
|
|
deleted = False
|
|
|
|
def read_file_raw(self, path):
|
|
return SimpleNamespace(
|
|
content="def old_func():\n return 42\n",
|
|
error=None,
|
|
)
|
|
|
|
def delete_file(self, path):
|
|
self.deleted = True
|
|
return SimpleNamespace(error=None)
|
|
|
|
file_ops = FakeFileOps()
|
|
result = apply_v4a_operations(ops, file_ops)
|
|
|
|
assert result.success is True
|
|
assert file_ops.deleted is True
|
|
# Diff must contain the actual removed lines, not a bare comment
|
|
assert "-def old_func():" in result.diff
|
|
assert "- return 42" in result.diff
|
|
assert "/dev/null" in result.diff
|
|
|
|
def test_delete_diff_fallback_on_empty_file(self):
|
|
"""An empty file should produce the fallback comment diff."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Delete File: empty.py
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is None
|
|
|
|
class FakeFileOps:
|
|
def read_file_raw(self, path):
|
|
return SimpleNamespace(content="", error=None)
|
|
|
|
def delete_file(self, path):
|
|
return SimpleNamespace(error=None)
|
|
|
|
result = apply_v4a_operations(ops, FakeFileOps())
|
|
assert result.success is True
|
|
# unified_diff produces nothing for two empty inputs — fallback comment expected
|
|
assert "Deleted" in result.diff or result.diff.strip() == ""
|
|
|
|
|
|
class TestCountOccurrences:
|
|
def test_basic(self):
|
|
from tools.patch_parser import _count_occurrences
|
|
assert _count_occurrences("aaa", "a") == 3
|
|
assert _count_occurrences("aaa", "aa") == 2
|
|
assert _count_occurrences("hello world", "xyz") == 0
|
|
assert _count_occurrences("", "x") == 0
|
|
|
|
|
|
class TestParseErrorSignalling:
|
|
"""Bug 3 regression tests — parse_v4a_patch must signal errors, not swallow them."""
|
|
|
|
def test_update_with_no_hunks_returns_error(self):
|
|
"""An UPDATE with no hunk lines is a malformed patch and should error."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: foo.py
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
assert err is not None, "Expected a parse error for hunk-less UPDATE"
|
|
assert ops == []
|
|
|
|
def test_move_without_destination_returns_error(self):
|
|
"""A MOVE without '->' syntax should not silently produce a broken operation."""
|
|
# The move regex requires '->' so this will be treated as an unrecognised
|
|
# line and the op is never created. Confirm nothing crashes and ops is empty.
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Move File: src/foo.py
|
|
*** End Patch"""
|
|
ops, err = parse_v4a_patch(patch)
|
|
# Either parse sees zero ops (fine) or returns an error (also fine).
|
|
# What is NOT acceptable is ops=[MOVE op with empty new_path] + err=None.
|
|
if ops:
|
|
assert err is not None, (
|
|
"MOVE with missing destination must either produce empty ops or an error"
|
|
)
|
|
|
|
def test_valid_patch_returns_no_error(self):
|
|
"""A well-formed patch must still return err=None."""
|
|
patch = """\
|
|
*** Begin Patch
|
|
*** Update File: f.py
|
|
ctx
|
|
-old
|
|
+new
|
|
*** End Patch"""
|
|
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 = (
|
|
"<diagnostics file=\"foo.ts\">\n"
|
|
"ERROR [1:7] some diagnostic\n"
|
|
"</diagnostics>"
|
|
)
|
|
|
|
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 = (
|
|
"<diagnostics file=\"bar.ts\">\n"
|
|
"ERROR [3:1] something\n"
|
|
"</diagnostics>"
|
|
)
|
|
|
|
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": "<diagnostics file=\"a.ts\">\nERR a\n</diagnostics>",
|
|
"b.ts": "<diagnostics file=\"b.ts\">\nERR b\n</diagnostics>",
|
|
}
|
|
|
|
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
|