From 02e6681177b6098f3dd7c4a4735f20854d2e3a66 Mon Sep 17 00:00:00 2001 From: Sergio Castillo Date: Fri, 5 Sep 2025 12:39:53 +0200 Subject: [PATCH] fix(#97): prevent suggestions on GitHub to start outside of diff hunk --- src/lgtm_ai/ai/prompts.py | 1 + src/lgtm_ai/formatters/markdown.py | 2 + .../formatters/templates/review_comment.md.j2 | 2 + src/lgtm_ai/git_parser/parser.py | 10 ++++ tests/formatters/test_json.py | 8 ++++ tests/formatters/test_markdown.py | 2 + tests/git_client/fixtures.py | 19 +++++++- tests/git_client/test_github.py | 8 ++++ tests/git_parser/fixtures.py | 42 ++++++++++++++--- tests/git_parser/test_parser.py | 47 ++++++++++++++++++- 10 files changed, 131 insertions(+), 10 deletions(-) diff --git a/src/lgtm_ai/ai/prompts.py b/src/lgtm_ai/ai/prompts.py index 8238bff..702585a 100644 --- a/src/lgtm_ai/ai/prompts.py +++ b/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: diff --git a/src/lgtm_ai/formatters/markdown.py b/src/lgtm_ai/formatters/markdown.py index eb9d32e..4ffd213 100644 --- a/src/lgtm_ai/formatters/markdown.py +++ b/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: diff --git a/src/lgtm_ai/formatters/templates/review_comment.md.j2 b/src/lgtm_ai/formatters/templates/review_comment.md.j2 index faf328f..13ea9e6 100644 --- a/src/lgtm_ai/formatters/templates/review_comment.md.j2 +++ b/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' }}` {% endif %} \ No newline at end of file diff --git a/src/lgtm_ai/git_parser/parser.py b/src/lgtm_ai/git_parser/parser.py index c1a80a3..2544cfd 100644 --- a/src/lgtm_ai/git_parser/parser.py +++ b/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 diff --git a/tests/formatters/test_json.py b/tests/formatters/test_json.py index be78d56..7a2dc49 100644 --- a/tests/formatters/test_json.py +++ b/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, } ], }, diff --git a/tests/formatters/test_markdown.py b/tests/formatters/test_markdown.py index 9654a32..d9731af 100644 --- a/tests/formatters/test_markdown.py +++ b/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`", "", "", ] diff --git a/tests/git_client/fixtures.py b/tests/git_client/fixtures.py index 47a8f91..163e781 100644 --- a/tests/git_client/fixtures.py +++ b/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, ), ], ), diff --git a/tests/git_client/test_github.py b/tests/git_client/test_github.py index 35384f3..787276f 100644 --- a/tests/git_client/test_github.py +++ b/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, ), ], ), diff --git a/tests/git_parser/fixtures.py b/tests/git_parser/fixtures.py index ca2ee2a..d8c1619 100644 --- a/tests/git_parser/fixtures.py +++ b/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) ''' diff --git a/tests/git_parser/test_parser.py b/tests/git_parser/test_parser.py index e1e4fad..a17874f 100644 --- a/tests/git_parser/test_parser.py +++ b/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}" + )