fix(#97): prevent suggestions on GitHub to start outside of diff hunk (#115)

Author: scastlara

src/lgtm_ai/ai/prompts.py

@@ -108,6 +108,7 @@ def _get_full_category_explanation() -> str:
     - Suggestions must be passed separately (not as part of the comment content), and they must include how many lines above and below the comment to include in the suggestion.
     - The offsets of suggestions must encompass all the code that needs to be changed. e.g., if you intend to change a whole function, the suggestion must include the full function. If you intend to change a single line, then the offsets will be 0.
     - If a suggestion is given, a flag indicating whether the suggestion is ready to be applied directly by the author must be given. That is, if the suggestion includes comments to be filled by the author, or skips parts and is intended for clarification, the flag `ready_for_replacement` must be set to `false`.
+    - Ensure that suggestions don't span outside git hunk boundaries (`hunk_start_new` and `hunk_start_old` in the modified lines; new for comments on new path, old for comments on old path). If they do, adjust the suggestion to fit within the hunk.
 
     The review will have a score for the PR (1-5, with 5 being the best). It is your job to evaluate whether this score holds after removing the comments.
     You must evaluate the score, and change it if necessary. Here is some guidance:

src/lgtm_ai/formatters/markdown.py

@@ -57,6 +57,8 @@ def format_review_comment(self, comment: ReviewComment, *, with_footer: bool = T
             new_path=comment.new_path,
             line_number=comment.line_number,
             relative_line_number=comment.relative_line_number,
+            with_suggestion=bool(comment.suggestion),
+            is_suggestion_ready=comment.suggestion and comment.suggestion.ready_for_replacement,
         )
 
     def format_guide(self, guide: ReviewGuide) -> str:

src/lgtm_ai/formatters/templates/review_comment.md.j2

@@ -26,5 +26,7 @@
 - **File**: `{{ new_path }}`
 - **Line**: `{{ line_number }}`
 - **Relative line**: `{{ relative_line_number }}`
+- **With suggestion**: `{{ 'Yes' if with_suggestion else 'No' }}`
+- **Suggestion ready for replacement**: `{{ 'Yes' if is_suggestion_ready else 'No' }}`
 </details>
 {% endif %}

src/lgtm_ai/git_parser/parser.py

@@ -10,6 +10,8 @@ class ModifiedLine(BaseModel):
     line_number: int
     relative_line_number: int
     modification_type: Literal["added", "removed"]
+    hunk_start_new: int | None = None
+    hunk_start_old: int | None = None
 
 
 class DiffFileMetadata(BaseModel):
@@ -37,6 +39,8 @@ def parse_diff_patch(metadata: DiffFileMetadata, diff_text: object) -> DiffResul
     old_line_num = 0
     new_line_num = 0
     rel_position = -1  # We just don't count the first hunk, but we do count the rest
+    hunk_start_old = None
+    hunk_start_new = None
 
     try:
         for line in lines:
@@ -45,6 +49,8 @@ def parse_diff_patch(metadata: DiffFileMetadata, diff_text: object) -> DiffResul
             if hunk_match:
                 old_line_num = int(hunk_match.group(1))
                 new_line_num = int(hunk_match.group(2))
+                hunk_start_new = new_line_num
+                hunk_start_old = old_line_num
                 continue
 
             if line.startswith("+") and not line.startswith("+++"):
@@ -54,6 +60,8 @@ def parse_diff_patch(metadata: DiffFileMetadata, diff_text: object) -> DiffResul
                         line_number=new_line_num,
                         relative_line_number=rel_position,
                         modification_type="added",
+                        hunk_start_new=hunk_start_new,
+                        hunk_start_old=hunk_start_old,
                     )
                 )
                 new_line_num += 1
@@ -65,6 +73,8 @@ def parse_diff_patch(metadata: DiffFileMetadata, diff_text: object) -> DiffResul
                         line_number=old_line_num,
                         relative_line_number=rel_position,
                         modification_type="removed",
+                        hunk_start_new=hunk_start_new,
+                        hunk_start_old=hunk_start_old,
                     )
                 )
                 old_line_num += 1

tests/formatters/test_json.py

@@ -56,6 +56,8 @@ def test_format_summary_section(self) -> None:
                                 "line_number": 2,
                                 "relative_line_number": 1,
                                 "modification_type": "removed",
+                                "hunk_start_new": None,
+                                "hunk_start_old": None,
                             }
                         ],
                     },
@@ -73,6 +75,8 @@ def test_format_summary_section(self) -> None:
                                 "line_number": 20,
                                 "relative_line_number": 2,
                                 "modification_type": "removed",
+                                "hunk_start_new": None,
+                                "hunk_start_old": None,
                             }
                         ],
                     },
@@ -149,6 +153,8 @@ def test_format_guide(self) -> None:
                                 "line_number": 2,
                                 "relative_line_number": 1,
                                 "modification_type": "removed",
+                                "hunk_start_new": None,
+                                "hunk_start_old": None,
                             }
                         ],
                     },
@@ -166,6 +172,8 @@ def test_format_guide(self) -> None:
                                 "line_number": 20,
                                 "relative_line_number": 2,
                                 "modification_type": "removed",
+                                "hunk_start_new": None,
+                                "hunk_start_old": None,
                             }
                         ],
                     },

tests/formatters/test_markdown.py

@@ -335,6 +335,8 @@ def test_format_comment_with_snippet(self) -> None:
             "- **File**: `new_path`",
             "- **Line**: `1`",
             "- **Relative line**: `1`",
+            "- **With suggestion**: `No`",
+            "- **Suggestion ready for replacement**: `No`",
             "</details>",
             "",
         ]

tests/git_client/fixtures.py

@@ -23,28 +23,43 @@
                 line_number=48,
                 relative_line_number=4,
                 modification_type="removed",
+                hunk_start_new=45,
+                hunk_start_old=45,
             ),
             ModifiedLine(
                 line='    {{ run }} ruff check {{ target_dirs }} {{ if report == "true" { "--output-format gitlab > tests/gl-code-quality-report.json" } else { "" } }}',
                 line_number=48,
                 relative_line_number=5,
                 modification_type="added",
+                hunk_start_new=45,
+                hunk_start_old=45,
             ),
         ],
     ),
     DiffResult(
         metadata=DiffFileMetadata(
-            new_file=False, deleted_file=False, renamed_file=False, new_path="pyproject.toml", old_path="pyproject.toml"
+            new_file=False,
+            deleted_file=False,
+            renamed_file=False,
+            new_path="pyproject.toml",
+            old_path="pyproject.toml",
         ),
         modified_lines=[
             ModifiedLine(
-                line="[tool.ruff.per-file-ignores]", line_number=78, relative_line_number=4, modification_type="removed"
+                line="[tool.ruff.per-file-ignores]",
+                line_number=78,
+                relative_line_number=4,
+                modification_type="removed",
+                hunk_start_new=75,
+                hunk_start_old=75,
             ),
             ModifiedLine(
                 line="[tool.ruff.lint.per-file-ignores]",
                 line_number=78,
                 relative_line_number=5,
                 modification_type="added",
+                hunk_start_new=75,
+                hunk_start_old=75,
             ),
         ],
     ),

tests/git_client/test_github.py

@@ -136,12 +136,16 @@ def test_get_diff_from_url_successful() -> None:
                         line_number=48,
                         relative_line_number=5,
                         modification_type="removed",
+                        hunk_start_new=48,
+                        hunk_start_old=48,
                     ),
                     ModifiedLine(
                         line='    {{ run }} ruff check {{ target_dirs }} {{ if report == "true" { "--output-format gitlab > tests/gl-code-quality-report.json" } else { "" } }}',
                         line_number=48,
                         relative_line_number=6,
                         modification_type="added",
+                        hunk_start_new=48,
+                        hunk_start_old=48,
                     ),
                 ],
             ),
@@ -159,12 +163,16 @@ def test_get_diff_from_url_successful() -> None:
                         line_number=78,
                         relative_line_number=5,
                         modification_type="removed",
+                        hunk_start_new=78,
+                        hunk_start_old=78,
                     ),
                     ModifiedLine(
                         line="[tool.ruff.lint.per-file-ignores]",
                         line_number=78,
                         relative_line_number=6,
                         modification_type="added",
+                        hunk_start_new=78,
+                        hunk_start_old=78,
                     ),
                 ],
             ),

tests/git_parser/fixtures.py

@@ -26,12 +26,16 @@
             line_number=48,
             relative_line_number=4,
             modification_type="removed",
+            hunk_start_new=45,
+            hunk_start_old=45,
         ),
         ModifiedLine(
             line='    {{ run }} ruff check {{ target_dirs }} {{ if report == "true" { "--output-format gitlab > tests/gl-code-quality-report.json" } else { "" } }}',
             line_number=48,
             relative_line_number=5,
             modification_type="added",
+            hunk_start_new=45,
+            hunk_start_old=45,
         ),
     ],
 )
@@ -49,17 +53,43 @@
 PARSED_REFACTOR_DIFF = DiffResult(
     metadata=DUMMY_METADATA,
     modified_lines=[
-        ModifiedLine(line="    for p in prices:", line_number=11, relative_line_number=2, modification_type="removed"),
-        ModifiedLine(line="        total += p", line_number=12, relative_line_number=3, modification_type="removed"),
         ModifiedLine(
-            line="    for price in prices:", line_number=11, relative_line_number=4, modification_type="added"
+            line="    for p in prices:",
+            line_number=11,
+            relative_line_number=2,
+            modification_type="removed",
+            hunk_start_new=10,
+            hunk_start_old=10,
+        ),
+        ModifiedLine(
+            line="        total += p",
+            line_number=12,
+            relative_line_number=3,
+            modification_type="removed",
+            hunk_start_new=10,
+            hunk_start_old=10,
+        ),
+        ModifiedLine(
+            line="    for price in prices:",
+            line_number=11,
+            relative_line_number=4,
+            modification_type="added",
+            hunk_start_new=10,
+            hunk_start_old=10,
+        ),
+        ModifiedLine(
+            line="        total += price",
+            line_number=12,
+            relative_line_number=5,
+            modification_type="added",
+            hunk_start_new=10,
+            hunk_start_old=10,
         ),
-        ModifiedLine(line="        total += price", line_number=12, relative_line_number=5, modification_type="added"),
     ],
 )
 
 COMPLEX_DIFF_TEXT = '''
-'@@ -2,7 +2,7 @@
+@@ -2,7 +2,7 @@
  import logging
  from collections.abc import Callable
  from importlib.metadata import version
@@ -180,5 +210,5 @@
 +    elif output_format == OutputFormat.json:
 +        return JsonFormatter(), print
 +    else:
-+        assert_never(output_format)'
++        assert_never(output_format)
 '''

tests/git_parser/test_parser.py

@@ -14,8 +14,8 @@
 @pytest.mark.parametrize(
     ("input_diff", "expected"),
     [
-        (SIMPLE_DIFF, PARSED_SIMPLE_DIFF),
-        (REFACTOR_DIFF, PARSED_REFACTOR_DIFF),
+        pytest.param(SIMPLE_DIFF, PARSED_SIMPLE_DIFF, id="simple"),
+        pytest.param(REFACTOR_DIFF, PARSED_REFACTOR_DIFF, id="refactor"),
     ],
 )
 def test_parse_diff_patch(input_diff: str, expected: DiffResult) -> None:
@@ -42,3 +42,46 @@ def test_parse_diff_relative_line_multiple_hunks(content: str, expected_line: in
     assert parsed_line.relative_line_number == expected_line, (
         f"Expected line number {expected_line}, got {parsed_line.line_number}"
     )
+
+
+def test_parse_diff_hunk_starts_set_correctly() -> None:
+    """Test that hunk_start_new and hunk_start_old are correctly set for all modified lines."""
+    parsed = parse_diff_patch(DUMMY_METADATA, COMPLEX_DIFF_TEXT)
+
+    # Check that no modified lines have None for hunk starts
+    for line in parsed.modified_lines:
+        assert line.hunk_start_new is not None, f"hunk_start_new is None for line: {line.line}"
+        assert line.hunk_start_old is not None, f"hunk_start_old is None for line: {line.line}"
+
+    # Test specific lines and their expected hunk starts
+    expected_hunk_starts = [
+        # First hunk @@ -2,7 +2,7 @@
+        ("from typing import get_args", 2, 2),
+        ("from typing import Any, assert_never, get_args", 2, 2),
+        # Second hunk @@ -13,10 +13,12 @@
+        ("from lgtm_ai.base.schemas import PRUrl", 13, 13),
+        ("from lgtm_ai.base.schemas import OutputFormat, PRUrl", 13, 13),
+        ("from lgtm_ai.formatters.base import Formatter", 13, 13),
+        # Third hunk @@ -25,14 +27,15 @@
+        ("from rich.console import Console", 25, 27),
+        # Fourth hunk @@ -68,6 +71,7 @@
+        ('@click.option("--output-format", type=click.Choice([format.value for format in OutputFormat]))', 68, 71),
+        # Last hunk @@ -220,3 +228,15 @@
+        (
+            "def _get_formatter_and_printer(output_format: OutputFormat) -> tuple[Formatter[Any], Callable[[Any], None]]:",
+            220,
+            228,
+        ),
+    ]
+
+    for content, expected_old, expected_new in expected_hunk_starts:
+        matching_lines = [line for line in parsed.modified_lines if content in line.line]
+        assert len(matching_lines) > 0, f"No line found containing: {content}"
+
+        line = matching_lines[0]
+        assert line.hunk_start_old == expected_old, (
+            f"Expected hunk_start_old {expected_old}, got {line.hunk_start_old} for line: {content}"
+        )
+        assert line.hunk_start_new == expected_new, (
+            f"Expected hunk_start_new {expected_new}, got {line.hunk_start_new} for line: {content}"
+        )