test: reorganize test structure and add missing unit tests
Reorganize flat tests/ directory to mirror source code structure
(tools/, gateway/, hermes_cli/, integration/). Add 11 new test files
covering previously untested modules: registry, patch_parser,
fuzzy_match, todo_tool, approval, file_tools, gateway session/config/
delivery, and hermes_cli config/models. Total: 147 unit tests passing,
9 integration tests gated behind pytest marker.
2026-02-26 03:20:08 +03:00
|
|
|
"""Tests for the V4A patch format parser."""
|
|
|
|
|
|
2026-03-14 03:54:46 -07:00
|
|
|
from types import SimpleNamespace
|
|
|
|
|
|
test: reorganize test structure and add missing unit tests
Reorganize flat tests/ directory to mirror source code structure
(tools/, gateway/, hermes_cli/, integration/). Add 11 new test files
covering previously untested modules: registry, patch_parser,
fuzzy_match, todo_tool, approval, file_tools, gateway session/config/
delivery, and hermes_cli config/models. Total: 147 unit tests passing,
9 integration tests gated behind pytest marker.
2026-02-26 03:20:08 +03:00
|
|
|
from tools.patch_parser import (
|
|
|
|
|
OperationType,
|
2026-03-14 03:54:46 -07:00
|
|
|
apply_v4a_operations,
|
test: reorganize test structure and add missing unit tests
Reorganize flat tests/ directory to mirror source code structure
(tools/, gateway/, hermes_cli/, integration/). Add 11 new test files
covering previously untested modules: registry, patch_parser,
fuzzy_match, todo_tool, approval, file_tools, gateway session/config/
delivery, and hermes_cli config/models. Total: 147 unit tests passing,
9 integration tests gated behind pytest marker.
2026-02-26 03:20:08 +03:00
|
|
|
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
|
2026-03-14 03:54:46 -07:00
|
|
|
|
|
|
|
|
|
|
|
|
|
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
|
|
|
|
|
|
fix(patch): harden V4A patch parser and fuzzy match — 9 correctness bugs
- Bug 1: replace read_file(limit=10000) with read_file_raw in _apply_update,
preventing silent truncation of files >2000 lines and corruption of lines
>2000 chars; add read_file_raw to FileOperations abstract interface and
ShellFileOperations
- Bug 2: split apply_v4a_operations into validate-then-apply phases; if any
hunk fails validation, zero writes occur (was: continue after failure,
leaving filesystem partially modified)
- Bug 3: parse_v4a_patch now returns an error for begin-marker-with-no-ops,
empty file paths, and moves missing a destination (was: always returned
error=None)
- Bug 4: raise strategy 7 (block anchor) single-candidate similarity threshold
from 0.10 to 0.50, eliminating false-positive matches in repetitive code
- Bug 5: add _strategy_unicode_normalized (new strategy 7) with position
mapping via _build_orig_to_norm_map; smart quotes and em-dashes in
LLM-generated patches now match via strategies 1-6 before falling through
to fuzzy strategies
- Bug 6: extend fuzzy_find_and_replace to return 4-tuple (content, count,
error, strategy); update all 5 call sites across patch_parser.py,
file_operations.py, and skill_manager_tool.py
- Bug 7: guard in _apply_update returns error when addition-only context hint
is ambiguous (>1 occurrences); validation phase errors on both 0 and >1
- Bug 8: _apply_delete returns error (not silent success) on missing file
- Bug 9: _validate_operations checks source existence and destination absence
for MOVE operations before any write occurs
2026-04-10 00:11:07 +02:00
|
|
|
def read_file_raw(self, path):
|
2026-03-14 03:54:46 -07:00
|
|
|
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'
|
|
|
|
|
)
|
2026-03-26 19:38:04 -07:00
|
|
|
|
|
|
|
|
|
|
|
|
|
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
|
fix(patch): harden V4A patch parser and fuzzy match — 9 correctness bugs
- Bug 1: replace read_file(limit=10000) with read_file_raw in _apply_update,
preventing silent truncation of files >2000 lines and corruption of lines
>2000 chars; add read_file_raw to FileOperations abstract interface and
ShellFileOperations
- Bug 2: split apply_v4a_operations into validate-then-apply phases; if any
hunk fails validation, zero writes occur (was: continue after failure,
leaving filesystem partially modified)
- Bug 3: parse_v4a_patch now returns an error for begin-marker-with-no-ops,
empty file paths, and moves missing a destination (was: always returned
error=None)
- Bug 4: raise strategy 7 (block anchor) single-candidate similarity threshold
from 0.10 to 0.50, eliminating false-positive matches in repetitive code
- Bug 5: add _strategy_unicode_normalized (new strategy 7) with position
mapping via _build_orig_to_norm_map; smart quotes and em-dashes in
LLM-generated patches now match via strategies 1-6 before falling through
to fuzzy strategies
- Bug 6: extend fuzzy_find_and_replace to return 4-tuple (content, count,
error, strategy); update all 5 call sites across patch_parser.py,
file_operations.py, and skill_manager_tool.py
- Bug 7: guard in _apply_update returns error when addition-only context hint
is ambiguous (>1 occurrences); validation phase errors on both 0 and >1
- Bug 8: _apply_delete returns error (not silent success) on missing file
- Bug 9: _validate_operations checks source existence and destination absence
for MOVE operations before any write occurs
2026-04-10 00:11:07 +02:00
|
|
|
def read_file_raw(self, path):
|
2026-03-26 19:38:04 -07:00
|
|
|
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
|
fix(patch): harden V4A patch parser and fuzzy match — 9 correctness bugs
- Bug 1: replace read_file(limit=10000) with read_file_raw in _apply_update,
preventing silent truncation of files >2000 lines and corruption of lines
>2000 chars; add read_file_raw to FileOperations abstract interface and
ShellFileOperations
- Bug 2: split apply_v4a_operations into validate-then-apply phases; if any
hunk fails validation, zero writes occur (was: continue after failure,
leaving filesystem partially modified)
- Bug 3: parse_v4a_patch now returns an error for begin-marker-with-no-ops,
empty file paths, and moves missing a destination (was: always returned
error=None)
- Bug 4: raise strategy 7 (block anchor) single-candidate similarity threshold
from 0.10 to 0.50, eliminating false-positive matches in repetitive code
- Bug 5: add _strategy_unicode_normalized (new strategy 7) with position
mapping via _build_orig_to_norm_map; smart quotes and em-dashes in
LLM-generated patches now match via strategies 1-6 before falling through
to fuzzy strategies
- Bug 6: extend fuzzy_find_and_replace to return 4-tuple (content, count,
error, strategy); update all 5 call sites across patch_parser.py,
file_operations.py, and skill_manager_tool.py
- Bug 7: guard in _apply_update returns error when addition-only context hint
is ambiguous (>1 occurrences); validation phase errors on both 0 and >1
- Bug 8: _apply_delete returns error (not silent success) on missing file
- Bug 9: _validate_operations checks source existence and destination absence
for MOVE operations before any write occurs
2026-04-10 00:11:07 +02:00
|
|
|
def read_file_raw(self, path):
|
2026-03-26 19:38:04 -07:00
|
|
|
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
|
fix(patch): harden V4A patch parser and fuzzy match — 9 correctness bugs
- Bug 1: replace read_file(limit=10000) with read_file_raw in _apply_update,
preventing silent truncation of files >2000 lines and corruption of lines
>2000 chars; add read_file_raw to FileOperations abstract interface and
ShellFileOperations
- Bug 2: split apply_v4a_operations into validate-then-apply phases; if any
hunk fails validation, zero writes occur (was: continue after failure,
leaving filesystem partially modified)
- Bug 3: parse_v4a_patch now returns an error for begin-marker-with-no-ops,
empty file paths, and moves missing a destination (was: always returned
error=None)
- Bug 4: raise strategy 7 (block anchor) single-candidate similarity threshold
from 0.10 to 0.50, eliminating false-positive matches in repetitive code
- Bug 5: add _strategy_unicode_normalized (new strategy 7) with position
mapping via _build_orig_to_norm_map; smart quotes and em-dashes in
LLM-generated patches now match via strategies 1-6 before falling through
to fuzzy strategies
- Bug 6: extend fuzzy_find_and_replace to return 4-tuple (content, count,
error, strategy); update all 5 call sites across patch_parser.py,
file_operations.py, and skill_manager_tool.py
- Bug 7: guard in _apply_update returns error when addition-only context hint
is ambiguous (>1 occurrences); validation phase errors on both 0 and >1
- Bug 8: _apply_delete returns error (not silent success) on missing file
- Bug 9: _validate_operations checks source existence and destination absence
for MOVE operations before any write occurs
2026-04-10 00:11:07 +02:00
|
|
|
|
|
|
|
|
|
|
|
|
|
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
|
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.
2026-05-20 01:46:40 -05:00
|
|
|
|
|
|
|
|
|
|
|
|
|
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
|