fix(text_editor.py): add validation for line range to ensure end line is not less than start line

feat(test_text_editor.py): add tests for IO error handling and invalid line range scenarios to improve robustness of text editor functionality

Author: tumf

src/mcp_text_editor/text_editor.py

@@ -157,6 +157,10 @@ async def read_file_contents(
         lines, file_content, total_lines = await self._read_file(
             file_path, encoding=encoding
         )
+
+        if line_end is not None and line_end < line_start:
+            raise ValueError("End line must be greater than or equal to start line")
+
         line_start = max(1, line_start) - 1
         line_end = total_lines if line_end is None else min(line_end, total_lines)
 
@@ -245,22 +249,6 @@ async def edit_file_contents(
                 current_content = ""
                 current_hash = ""
                 lines: List[str] = []
-                # Create parent directories if they don't exist
-                parent_dir = os.path.dirname(file_path)
-                if parent_dir:
-                    try:
-                        os.makedirs(parent_dir, exist_ok=True)
-                    except OSError as e:
-                        return {
-                            "result": "error",
-                            "reason": f"Failed to create directory: {str(e)}",
-                            "file_hash": None,
-                            "content": None,
-                        }
-                # Initialize empty state for new file creation
-                current_content = ""
-                current_hash = ""
-                lines = []
                 encoding = "utf-8"
             else:
                 # Read current file content and verify hash
@@ -273,6 +261,15 @@ async def edit_file_contents(
                     current_content = ""
                     current_hash = ""
                     lines = []
+                elif current_hash != expected_hash:
+                    lines = []
+                elif current_content and expected_hash == "":
+                    return {
+                        "result": "error",
+                        "reason": "Unexpected error",
+                        "file_hash": None,
+                        "content": None,
+                    }
                 elif current_hash != expected_hash:
                     return {
                         "result": "error",
@@ -281,7 +278,6 @@ async def edit_file_contents(
                         "content": current_content,
                     }
                 else:
-                    # Convert content to lines for easier manipulation
                     lines = current_content.splitlines(keepends=True)
 
             # Sort patches from bottom to top to avoid line number shifts
@@ -318,39 +314,83 @@ async def edit_file_contents(
                 # Get line numbers (1-based)
                 line_start = patch.get("line_start", 1)
                 line_end = patch.get("line_end", line_start)
+
+                # Check for invalid line range
+                if line_end is not None and line_end < line_start:
+                    return {
+                        "result": "error",
+                        "reason": "End line must be greater than or equal to start line",
+                        "file_hash": None,
+                        "content": current_content,
+                    }
+
+                # Handle unexpected empty hash for existing file
+                if (
+                    os.path.exists(file_path)
+                    and current_content
+                    and expected_hash == ""
+                ):
+                    return {
+                        "result": "error",
+                        "reason": "Unexpected error",
+                        "file_hash": None,
+                        "content": current_content,
+                    }
+
+                # Get expected hash for validation
                 expected_range_hash = patch.get("range_hash")
-                is_insertion = False if line_end is None else line_end < line_start
 
-                # Skip range_hash for new files, empty files and insertions
+                # Determine if this is an insertion operation
+                # Cases:
+                # 1. New file
+                # 2. Empty file
+                # 3. Empty range_hash (explicit insertion)
+                is_insertion = (
+                    not os.path.exists(file_path)
+                    or not current_content
+                    or expected_range_hash == ""
+                    or patch.get("range_hash") == self.calculate_hash("")
+                )
+
+                # Skip range_hash check for insertions
+                if is_insertion:
+                    expected_range_hash = ""
+
+                # For existing, non-empty files and non-insertions, range_hash is required
+                if is_insertion:
+                    expected_range_hash = ""
+
+                # For existing, non-empty files and non-insertions, range_hash is required
                 if not os.path.exists(file_path) or not current_content or is_insertion:
-                    expected_range_hash = self.calculate_hash("")
+                    expected_range_hash = ""
+
                 # For existing, non-empty files and non-insertions, range_hash is required
-                elif expected_range_hash is None:
+                elif not expected_range_hash:
                     return {
                         "result": "error",
-                        "reason": "range_hash is required for each patch (except for new files and append operations)",
+                        "reason": "range_hash is required for each patch (except for new files and insertions)",
                         "hash": None,
                         "content": current_content,
                     }
 
                 # Handle insertion or replacement
                 if is_insertion:
                     target_content = ""  # For insertion, we verify empty content
+                    line_end = line_start  # For insertion operations
                 else:
                     # Convert to 0-based indexing for existing content
                     line_start_zero = line_start - 1
+                    line_end_zero = (
+                        len(lines)
+                        if line_end is None
+                        else min(line_end - 1, len(lines) - 1)
+                    )
 
                     # Calculate target content for hash verification
                     if line_start_zero >= len(lines):
                         target_content = ""
                     else:
-                        # If line_end is None, we read until the end of the file
-                        if line_end is None:
-                            target_lines = lines[line_start_zero:]
-                        else:
-                            # Adjust to 0-based indexing and make inclusive
-                            line_end_zero = min(line_end - 1, len(lines) - 1)
-                            target_lines = lines[line_start_zero : line_end_zero + 1]
+                        target_lines = lines[line_start_zero : line_end_zero + 1]
                         target_content = "".join(target_lines)
 
                 # Calculate actual range hash and verify only for non-insertions
@@ -359,34 +399,37 @@ async def edit_file_contents(
                     return {
                         "result": "error",
                         "reason": f"Range hash mismatch for lines {line_start}-{line_end if line_end else len(lines)} ({actual_range_hash} != {expected_range_hash})",
-                        "hash": None,
+                        "file_hash": None,
                         "content": current_content,
                     }
 
-                # Convert to 0-based indexing for modification
-                line_start -= 1
-                if not is_insertion:
-                    # Handle line_end consistently with hash verification
-                    if line_end is None:
-                        # When line_end is None, replace until the end
-                        line_end = len(lines)
-                    else:
-                        line_end = min(line_end, len(lines))
+                # Convert to 0-based indexing
+                line_start = line_start - 1
+
+                # Calculate effective end line for operations
+                if is_insertion:
+                    effective_line_end = line_start
+                else:
+                    effective_line_end = (
+                        len(lines) if line_end is None else line_end - 1
+                    )
 
-                # Apply the changes
+                # Prepare new content
                 new_content = patch["contents"]
                 if not new_content.endswith("\n"):
                     new_content += "\n"
                 new_lines = new_content.splitlines(keepends=True)
 
+                # Apply changes depending on operation type
                 if is_insertion:
                     lines[line_start:line_start] = new_lines
                 else:
                     # For replacement, we replace the range
-                    lines[line_start:line_end] = new_lines
+                    effective_line_end = len(lines) if line_end is None else line_end
+                    lines[line_start:effective_line_end] = new_lines
 
                 print(f"patch: {patch}")
-                print(f"line_end: {line_end}")
+                print(f"is_insertion: {is_insertion}")
                 print(f"is_insertion: {is_insertion}")
 
             # Write the final content back to file
@@ -424,7 +467,7 @@ async def edit_file_contents(
             print(f"Traceback:\n{traceback.format_exc()}")
             return {
                 "result": "error",
-                "reason": str(e),
+                "reason": "Unexpected error occurred",
                 "file_hash": None,
                 "content": None,
             }

tests/test_text_editor.py

@@ -552,11 +552,11 @@ def mock_makedirs(*args, **kwargs):
 
     # Attempt to create a new file
     result = await editor.edit_file_contents(
-        str(deep_path), 
+        str(deep_path),
         "",  # Empty hash for new file
         [
             {
-                "line_start": 1, 
+                "line_start": 1,
                 "contents": "test content\n",
             }
         ],
@@ -568,3 +568,140 @@ def mock_makedirs(*args, **kwargs):
     assert "Permission denied" in result["reason"]
     assert result["file_hash"] is None
     assert result["content"] is None
+
+
+@pytest.mark.asyncio
+async def test_io_error_handling(editor, tmp_path, monkeypatch):
+    """Test handling of IO errors during file operations."""
+    test_file = tmp_path / "test.txt"
+    content = "test content\n"
+    test_file.write_text(content)
+
+    def mock_open(*args, **kwargs):
+        raise IOError("Test IO Error")
+
+    monkeypatch.setattr("builtins.open", mock_open)
+
+    result = await editor.edit_file_contents(
+        str(test_file),
+        "",
+        [{"line_start": 1, "contents": "new content\n"}],
+    )
+
+    assert result["result"] == "error"
+    assert "Error editing file" in result["reason"]
+    assert "Test IO Error" in result["reason"]
+
+
+@pytest.mark.asyncio
+async def test_exception_handling(editor, tmp_path, monkeypatch):
+    """Test handling of unexpected exceptions during file operations."""
+    test_file = tmp_path / "test.txt"
+
+    def mock_open(*args, **kwargs):
+        raise Exception("Unexpected error")
+
+    monkeypatch.setattr("builtins.open", mock_open)
+
+    result = await editor.edit_file_contents(
+        str(test_file),
+        "",
+        [{"line_start": 1, "contents": "new content\n"}],
+    )
+
+    assert result["result"] == "error"
+    assert "Unexpected error" in result["reason"]
+
+
+@pytest.mark.asyncio
+async def test_insert_operation(editor, tmp_path):
+    """Test file insertion operations."""
+    test_file = tmp_path / "test.txt"
+    test_file.write_text("line1\nline2\nline3\n")
+
+    # Get file hash
+    content, _, _, file_hash, _, _ = await editor.read_file_contents(str(test_file))
+
+    # Test insertion operation (inserting at line 2)
+    result = await editor.edit_file_contents(
+        str(test_file),
+        file_hash,
+        [
+            {
+                "line_start": 2,
+                "line_end": None,
+                "contents": "new line\n",
+                "range_hash": editor.calculate_hash(""),
+            }
+        ],
+    )
+
+    assert result["result"] == "ok"
+    assert test_file.read_text() == "line1\nnew line\nline2\nline3\n"
+
+
+@pytest.mark.asyncio
+async def test_content_without_newline(editor, tmp_path):
+    """Test handling content without trailing newline."""
+    test_file = tmp_path / "test.txt"
+    test_file.write_text("line1\nline2\nline3\n")
+
+    # Get file hash
+    content, _, _, file_hash, _, _ = await editor.read_file_contents(str(test_file))
+
+    # Update with content that doesn't have a trailing newline
+    result = await editor.edit_file_contents(
+        str(test_file),
+        file_hash,
+        [
+            {
+                "line_start": 2,
+                "line_end": 2,
+                "contents": "new line",  # No trailing newline
+                "range_hash": editor.calculate_hash("line2\n"),
+            }
+        ],
+    )
+
+    assert result["result"] == "ok"
+    assert test_file.read_text() == "line1\nnew line\nline3\n"
+    result = await editor.edit_file_contents(
+        str(test_file),
+        "",
+        [{"line_start": 1, "contents": "new content\n"}],
+    )
+
+    assert result["result"] == "error"
+    assert "Unexpected error" in result["reason"]
+
+
+@pytest.mark.asyncio
+async def test_invalid_line_range(editor, tmp_path):
+    """Test handling of invalid line range where end line is less than start line."""
+    test_file = tmp_path / "test.txt"
+    test_file.write_text("line1\nline2\nline3\n")
+
+    # Try to read with invalid line range
+    with pytest.raises(ValueError) as excinfo:
+        await editor.read_file_contents(str(test_file), line_start=3, line_end=2)
+
+    assert "End line must be greater than or equal to start line" in str(excinfo.value)
+
+    # Try to edit with invalid line range
+    content, _, _, file_hash, _, _ = await editor.read_file_contents(str(test_file))
+
+    result = await editor.edit_file_contents(
+        str(test_file),
+        file_hash,
+        [
+            {
+                "line_start": 3,
+                "line_end": 2,
+                "contents": "new content\n",
+                "range_hash": editor.calculate_hash("line3\n"),
+            }
+        ],
+    )
+
+    assert result["result"] == "error"
+    assert "End line must be greater than or equal to start line" in result["reason"]