Merge branch 'feature/required-end-parameter' into develop

Author: tumfwkm

src/mcp_text_editor/models.py

@@ -2,7 +2,7 @@
 
 from typing import Dict, List, Optional
 
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, Field, model_validator
 
 
 class GetTextFileContentsRequest(BaseModel):
@@ -29,9 +29,22 @@ class EditPatch(BaseModel):
     end: Optional[int] = Field(None, description="Ending line for edit")
     contents: str = Field(..., description="New content to insert")
     range_hash: Optional[str] = Field(
-        None, description="Hash of content being replaced. None for insertions"
+        None,  # None for new patches, must be explicitly set
+        description="Hash of content being replaced. Empty string for insertions.",
     )
 
+    @model_validator(mode="after")
+    def validate_end_line(self) -> "EditPatch":
+        """Validate that end line is present when not in append mode."""
+        # range_hash must be explicitly set
+        if self.range_hash is None:
+            raise ValueError("range_hash is required")
+
+        # For modifications (non-empty range_hash), end is required
+        if self.range_hash != "" and self.end is None:
+            raise ValueError("end line is required when not in append mode")
+        return self
+
 
 class EditFileOperation(BaseModel):
     """Model for individual file edit operation."""

src/mcp_text_editor/text_editor.py

@@ -1,11 +1,14 @@
 """Core text editor functionality with file operation handling."""
 
 import hashlib
+import logging
 import os
 from typing import Any, Dict, List, Optional, Tuple
 
 from mcp_text_editor.models import EditPatch, FileRanges
 
+logger = logging.getLogger(__name__)
+
 
 class TextEditor:
     """Handles text file operations with security checks and conflict detection."""
@@ -338,9 +341,6 @@ async def edit_file_contents(
 
                 # Calculate line ranges for zero-based indexing
                 start_zero = start - 1
-                end_zero = (
-                    len(lines) - 1 if end is None else min(end - 1, len(lines) - 1)
-                )
 
                 # Get expected hash for validation
                 expected_range_hash = None
@@ -358,49 +358,30 @@ async def edit_file_contents(
                     # Append mode - start exceeds total lines
                     is_insertion = True
                 else:
-                    # For existing files:
-                    # range_hash is required for modifications
-                    if not expected_range_hash:
-                        return {
-                            "result": "error",
-                            "reason": "range_hash is required for file modifications",
-                            "file_hash": None,
-                            "content": current_content,
-                        }
-
-                    # Hash provided - verify content
-                    target_lines = lines[start_zero : end_zero + 1]
-                    target_content = "".join(target_lines)
-                    actual_range_hash = self.calculate_hash(target_content)
-
-                    # Debug output for hash comparison
-                    print(
-                        f"Debug - Range hash comparison:"
-                        f"\nExpected: {expected_range_hash}"
-                        f"\nActual: {actual_range_hash}"
-                        f"\nContent: {target_content!r}"
-                        f"\nRange: {start_zero}:{end_zero + 1}"
-                    )
-
-                    # Compare hashes
-                    # Empty range_hash means explicit insertion
-                    is_insertion = (
-                        not expected_range_hash
-                        or expected_range_hash == self.calculate_hash("")
-                    )
-
-                    # For non-insertion operations, verify content hash
+                    # For modification mode, check the range_hash
+                    is_insertion = expected_range_hash == ""
                     if not is_insertion:
+                        # Calculate end_zero for content validation
+                        end_zero = (
+                            len(lines) - 1
+                            if end is None
+                            else min(end - 1, len(lines) - 1)
+                        )
+
+                        # Hash provided - verify content
+                        target_lines = lines[start_zero : end_zero + 1]
+                        target_content = "".join(target_lines)
+                        actual_range_hash = self.calculate_hash(target_content)
+
                         if actual_range_hash != expected_range_hash:
                             return {
                                 "result": "error",
-                                "reason": f"Content hash mismatch - Please use get_text_file_contents tool with lines {start}-{end} to get current content and hashes, then retry with the updated hashes. If you want to append content, set start to {len(lines)+1}.",
+                                "reason": "Content range hash mismatch - Please use get_text_file_contents tool to get current content and hashes, then retry with the updated hashes.",
                                 "file_hash": None,
                                 "content": current_content,
                             }
 
                 # Prepare new content
-                contents: str
                 if isinstance(patch, EditPatch):
                     contents = patch.contents
                 else:
@@ -409,19 +390,14 @@ async def edit_file_contents(
                 new_content = contents if contents.endswith("\n") else contents + "\n"
                 new_lines = new_content.splitlines(keepends=True)
 
-                # Apply changes - line ranges were calculated earlier
+                # For insertion mode, we don't need end_zero
                 if is_insertion:
                     # Insert at the specified line
                     lines[start_zero:start_zero] = new_lines
                 else:
-                    # Replace the specified range
+                    # We already have end_zero for replacements
                     lines[start_zero : end_zero + 1] = new_lines
 
-                # Debug output - shows the operation type
-                print(
-                    f"Applied patch: start={start} end={end} is_insertion={is_insertion} contents={patch.contents!r}"
-                )
-
             # Write the final content back to file
             final_content = "".join(lines)
             with open(file_path, "w", encoding=encoding) as f:
@@ -453,8 +429,8 @@ async def edit_file_contents(
         except Exception as e:
             import traceback
 
-            print(f"Error: {str(e)}")
-            print(f"Traceback:\n{traceback.format_exc()}")
+            logger.error(f"Error: {str(e)}")
+            logger.error(f"Traceback:\n{traceback.format_exc()}")
             return {
                 "result": "error",
                 "reason": "Unexpected error occurred",

tests/test_models.py

@@ -51,28 +51,41 @@ def test_get_text_file_contents_response():
 
 def test_edit_patch():
     """Test EditPatch model."""
-    # Test with only required field
-    patch = EditPatch(contents="new content")
+    # Test that range_hash is required
+    with pytest.raises(ValueError, match="range_hash is required"):
+        EditPatch(contents="new content")
+    with pytest.raises(ValueError, match="range_hash is required"):
+        EditPatch(contents="new content", start=1)
+
+    # Test append mode with empty range_hash
+    patch = EditPatch(contents="new content", start=1, range_hash="")
     assert patch.contents == "new content"
-    assert patch.start == 1  # Default value
-    assert patch.end is None  # Default value
+    assert patch.start == 1
+    assert patch.end is None
 
-    # Test with all fields
-    patch = EditPatch(start=5, end=10, contents="new content")
+    # Test modification mode (requires end when range_hash is present)
+    patch = EditPatch(start=5, end=10, contents="new content", range_hash="somehash")
     assert patch.start == 5
     assert patch.end == 10
     assert patch.contents == "new content"
+    assert patch.range_hash == "somehash"
 
     # Test validation error - missing required field
     with pytest.raises(ValidationError):
         EditPatch()
 
+    # Test validation error - missing end in modification mode
+    with pytest.raises(
+        ValueError, match="end line is required when not in append mode"
+    ):
+        EditPatch(start=1, contents="content", range_hash="somehash")
+
 
 def test_edit_file_operation():
     """Test EditFileOperation model."""
     patches = [
-        EditPatch(contents="content1"),
-        EditPatch(start=2, end=3, contents="content2"),
+        EditPatch(contents="content1", range_hash=""),  # append mode
+        EditPatch(start=2, end=3, contents="content2", range_hash="somehash"),
     ]
     operation = EditFileOperation(
         path="/path/to/file.txt", hash="hash123", patches=patches
@@ -81,7 +94,9 @@ def test_edit_file_operation():
     assert operation.hash == "hash123"
     assert len(operation.patches) == 2
     assert operation.patches[0].contents == "content1"
+    assert operation.patches[0].range_hash == ""  # append mode
     assert operation.patches[1].start == 2
+    assert operation.patches[1].range_hash == "somehash"  # modification mode
 
     # Test validation error - missing required fields
     with pytest.raises(ValidationError):
@@ -123,7 +138,7 @@ def test_edit_text_file_contents_request():
             EditFileOperation(
                 path="/path/to/file.txt",
                 hash="hash123",
-                patches=[EditPatch(contents="new content")],
+                patches=[EditPatch(contents="new content", range_hash="")],
             )
         ]
     )
@@ -139,12 +154,12 @@ def test_edit_text_file_contents_request():
             EditFileOperation(
                 path="/path/to/file1.txt",
                 hash="hash123",
-                patches=[EditPatch(contents="content1")],
+                patches=[EditPatch(contents="content1", range_hash="")],
             ),
             EditFileOperation(
                 path="/path/to/file2.txt",
                 hash="hash456",
-                patches=[EditPatch(start=2, contents="content2")],
+                patches=[EditPatch(start=2, contents="content2", range_hash="")],
             ),
         ]
     )

tests/test_service.py

@@ -49,20 +49,20 @@ def test_validate_patches(service):
     """Test patch validation."""
     # Valid patches
     patches = [
-        EditPatch(start=1, end=2, contents="content1"),
-        EditPatch(start=3, end=4, contents="content2"),
+        EditPatch(start=1, end=2, contents="content1", range_hash="hash1"),
+        EditPatch(start=3, end=4, contents="content2", range_hash="hash2"),
     ]
     assert service.validate_patches(patches, 5) is True
 
     # Overlapping patches
     patches = [
-        EditPatch(start=1, end=3, contents="content1"),
-        EditPatch(start=2, end=4, contents="content2"),
+        EditPatch(start=1, end=3, contents="content1", range_hash="hash1"),
+        EditPatch(start=2, end=4, contents="content2", range_hash="hash2"),
     ]
     assert service.validate_patches(patches, 5) is False
 
     # Out of bounds patches
-    patches = [EditPatch(start=1, end=10, contents="content1")]
+    patches = [EditPatch(start=1, end=10, contents="content1", range_hash="hash1")]
     assert service.validate_patches(patches, 5) is False
 
 
@@ -81,7 +81,7 @@ def test_edit_file_contents(service, tmp_path):
     operation = EditFileOperation(
         path=file_path,
         hash=initial_hash,
-        patches=[EditPatch(start=2, end=2, contents="new line2")],
+        patches=[EditPatch(start=2, end=2, contents="new line2", range_hash="hash1")],
     )
 
     # Apply edit
@@ -108,7 +108,7 @@ def test_edit_file_contents_hash_mismatch(service, tmp_path):
     operation = EditFileOperation(
         path=file_path,
         hash="incorrect_hash",
-        patches=[EditPatch(start=2, end=2, contents="new line2")],
+        patches=[EditPatch(start=2, end=2, contents="new line2", range_hash="hash1")],
     )
 
     # Attempt edit
@@ -135,7 +135,9 @@ def test_edit_file_contents_invalid_patches(service, tmp_path):
         path=file_path,
         hash=initial_hash,
         patches=[
-            EditPatch(start=1, end=10, contents="new content")  # Beyond file length
+            EditPatch(
+                start=1, end=10, contents="new content", range_hash="hash1"
+            )  # Beyond file length
         ],
     )
 
@@ -152,7 +154,9 @@ def test_edit_file_contents_file_error(service):
     file_path = "nonexistent.txt"
     # Attempt to edit non-existent file
     operation = EditFileOperation(
-        path=file_path, hash="any_hash", patches=[EditPatch(contents="new content")]
+        path=file_path,
+        hash="any_hash",
+        patches=[EditPatch(contents="new content", range_hash="")],
     )
 
     # Attempt edit
@@ -170,7 +174,7 @@ def test_edit_file_unexpected_error(service, tmp_path):
     operation = EditFileOperation(
         path=test_file,
         hash="dummy_hash",
-        patches=[EditPatch(contents="test content\n", line_start=1)],
+        patches=[EditPatch(contents="test content\n", start=1, range_hash="")],
     )
 
     # Try to edit non-existent file
@@ -198,7 +202,7 @@ def test_edit_file_contents_permission_error(service, tmp_path):
     operation = EditFileOperation(
         path=file_path,
         hash=service.calculate_hash(test_content),
-        patches=[EditPatch(line_start=2, line_end=2, contents="new line2")],
+        patches=[EditPatch(start=2, end=2, contents="new line2", range_hash="hash1")],
     )
 
     # Attempt edit
@@ -219,7 +223,7 @@ def test_edit_file_contents_general_exception(service, mocker):
     operation = EditFileOperation(
         path=test_file,
         hash="hash123",
-        patches=[EditPatch(contents="new content", start=1)],
+        patches=[EditPatch(contents="new content", start=1, range_hash="")],
     )
 
     # Mock edit_file to raise an exception