diff --git a/bugbug/tools/code_review/__init__.py b/bugbug/tools/code_review/__init__.py index 3d9a28481b..6add43b189 100644 --- a/bugbug/tools/code_review/__init__.py +++ b/bugbug/tools/code_review/__init__.py @@ -30,8 +30,7 @@ # Exceptions (backward compatibility) from bugbug.tools.core.exceptions import ( - FileNotInPatchError, - HunkNotInPatchError, + CommentNotLocatedError, LargeDiffError, ModelResultError, ) @@ -56,8 +55,7 @@ "InlineComment", "ReviewRequest", # Exceptions - "FileNotInPatchError", - "HunkNotInPatchError", + "CommentNotLocatedError", "LargeDiffError", "ModelResultError", # Base classes diff --git a/bugbug/tools/code_review/agent.py b/bugbug/tools/code_review/agent.py index 0520f6b1a8..bc1ee8bf7b 100644 --- a/bugbug/tools/code_review/agent.py +++ b/bugbug/tools/code_review/agent.py @@ -49,9 +49,11 @@ ) from bugbug.tools.code_review.utils import ( convert_generated_comments_to_inline, + find_line_text, format_patch_set, ) from bugbug.tools.core.exceptions import ( + CommentNotLocatedError, LargeDiffError, RecursionLimitError, ) @@ -358,12 +360,31 @@ def _get_comment_examples(self, patch, created_before: datetime | None = None): for example in comment_examples: example["comment"]["explanation"] = "THE JUSTIFICATION GOES HERE" - def format_comment(comment): + def format_comment(example): # TODO: change the schema that we expect the model to return so we # can remove this function. + comment = example["comment"] + filename = comment["filename"] + raw_hunk = example.get("hunk") or example.get("raw_hunk") + existing_code = None + if raw_hunk: + try: + wrapped = TEMPLATE_PATCH_FROM_HUNK.format( + filename=filename, raw_hunk=raw_hunk + ) + patched_file = PatchSet.from_string(wrapped)[0] + existing_code = find_line_text(patched_file, comment["start_line"]) + except (CommentNotLocatedError, IndexError): + logger.warning( + "Could not recover existing_code for example comment on %s:%s", + filename, + comment["start_line"], + ) + if existing_code is None: + return None return { - "file": comment["filename"], - "code_line": comment["start_line"], + "file": filename, + "existing_code": existing_code, "comment": comment["content"], } @@ -375,10 +396,14 @@ def generate_formatted_patch_from_raw_hunk(raw_hunk, filename): return format_patch_set(patch_set) if not self.show_patch_example: - return json.dumps( - [format_comment(example["comment"]) for example in comment_examples], - indent=2, - ) + formatted_comments = [ + comment + for comment in ( + format_comment(example) for example in comment_examples + ) + if comment is not None + ] + return json.dumps(formatted_comments, indent=2) return "\n\n".join( TEMPLATE_COMMENT_EXAMPLE.format( @@ -387,7 +412,11 @@ def generate_formatted_patch_from_raw_hunk(raw_hunk, filename): example["raw_hunk"], example["comment"]["filename"] ), comments=json.dumps( - [format_comment(example["comment"])], + [ + comment + for comment in [format_comment(example)] + if comment is not None + ], indent=2, ), ) diff --git a/bugbug/tools/code_review/data_types.py b/bugbug/tools/code_review/data_types.py index 07d2c002fb..9d8084f20c 100644 --- a/bugbug/tools/code_review/data_types.py +++ b/bugbug/tools/code_review/data_types.py @@ -12,7 +12,14 @@ class GeneratedReviewComment(BaseModel): """A review comment generated by the code review agent.""" file: str = Field(description="The path to the file the comment applies to.") - code_line: int = Field(description="The line number that the comment refers to.") + existing_code: str = Field( + description=( + "The exact source line(s) the comment is about, copied verbatim " + "from the patch (leading '+'/'-'/' ' markers may be included or " + "omitted). Used to locate the comment in the diff, so do not " + "paraphrase, reformat, or truncate it." + ) + ) comment: str = Field(description="The review comment.") explanation: str = Field( description="A brief rationale for the comment, including how confident you are and why." diff --git a/bugbug/tools/code_review/prompts.py b/bugbug/tools/code_review/prompts.py index 59adc4bdf2..5e802f2479 100644 --- a/bugbug/tools/code_review/prompts.py +++ b/bugbug/tools/code_review/prompts.py @@ -44,6 +44,7 @@ - Use directive language: "Fix", "Remove", "Change", "Add" - NEVER use these banned phrases: "maybe", "might want to", "consider", "possibly", "could be", "you may want to" - Focus strictly on code-related concerns +- For `existing_code`, copy the exact source line(s) the comment is about verbatim from the patch — do not paraphrase, reformat, or guess a line number instead ## What NOT to Include @@ -72,7 +73,7 @@ - A large change that has no natural seam and must land atomically is acceptable — return an empty list rather than suggesting an impractical split. - If you do comment, name concrete seams: the distinct concerns, or the stages of a large cohesive change (e.g. land the data-model change separately from the call-site updates). - Briefly tell the author *why*: larger patches get less thorough review and empirically introduce more bugs and regressions, so smaller patches are easier to review and safer to land. -- Anchor the comment to a representative changed line (a line that begins with `+`). Set `file` to that file's path and `code_line` to that line's number. +- Anchor the comment to a representative changed line (a line that begins with `+`). Set `file` to that file's path and `existing_code` to that line's exact text, copied verbatim. - Use direct, declarative language. NEVER use these banned phrases: "maybe", "might want to", "consider", "possibly", "could be", "you may want to". Here is a summary of the patch: diff --git a/bugbug/tools/code_review/utils.py b/bugbug/tools/code_review/utils.py index f242111b87..8ff9e1a1b3 100644 --- a/bugbug/tools/code_review/utils.py +++ b/bugbug/tools/code_review/utils.py @@ -5,54 +5,118 @@ import re -from itertools import chain from logging import getLogger from typing import Iterable from unidiff import Hunk, PatchedFile, PatchSet from bugbug.tools.core.data_types import InlineComment -from bugbug.tools.core.exceptions import ( - FileNotInPatchError, - HunkNotInPatchError, - ModelResultError, -) +from bugbug.tools.core.exceptions import CommentNotLocatedError logger = getLogger(__name__) -def find_comment_scope(file: PatchedFile, line_number: int): - hunks_based_on_added = ( - hunk - for hunk in file - if hunk.target_start <= line_number <= hunk.target_start + hunk.target_length - ) - hunks_based_on_deleted = ( - hunk - for hunk in file - if hunk.source_start <= line_number <= hunk.source_start + hunk.source_length - ) +def _normalize_line(line: str) -> str: + """Strip whitespace and an optional leading diff marker from a line.""" + line = line.strip() + if line[:1] in ("+", "-"): + line = line[1:] + return line.strip() - try: - hunk = next(chain(hunks_based_on_added, hunks_based_on_deleted)) - except StopIteration as e: - raise HunkNotInPatchError("Line number not found in the patch") from e - has_added_lines = any(line.is_added for line in hunk) - has_deleted_lines = any(line.is_removed for line in hunk) +def _side_lines(hunk: Hunk, new_side: bool) -> list[tuple[int, str]]: + """Extract one side of a hunk as (line_number, normalized_content) pairs. - if has_added_lines and has_deleted_lines: - first_line, last_line = find_mixed_lines_range(hunk) - elif has_added_lines: - first_line, last_line = find_added_lines_range(hunk) - else: - first_line, last_line = find_removed_lines_range(hunk) + new_side=True: context + added lines, numbered against the new file. + new_side=False: context + removed lines, numbered against the old file. + """ + result = [] + for line in hunk: + if new_side: + if line.is_context or line.is_added: + result.append((line.target_line_no, _normalize_line(line.value))) + elif line.is_context or line.is_removed: + result.append((line.source_line_no, _normalize_line(line.value))) + return result + + +def _match_consecutive( + side_lines: list[tuple[int, str]], target_lines: list[str] +) -> tuple[int, int] | None: + """Find a consecutive run in `side_lines` matching `target_lines`, if any.""" + n = len(target_lines) + if n == 0 or len(side_lines) < n: + return None + for i in range(len(side_lines) - n + 1): + if all(side_lines[i + j][1] == target_lines[j] for j in range(n)): + return side_lines[i][0], side_lines[i + n - 1][0] + return None - return { - "line_start": first_line, - "line_end": last_line, - "has_added_lines": has_added_lines, - } + +def find_comment_location(file: PatchedFile, existing_code: str) -> dict: + """Locate the line range in `file` matching `existing_code` verbatim. + + Rather than trusting a model-provided line number (which requires + counting from a hunk header and is error-prone), this matches the quoted + snippet against the actual patch content and derives line numbers + deterministically — the approach used by alibaba/open-code-review. Tries + the new side (context + added lines) first, then the old side (context + + removed lines), across all hunks. + + Raises CommentNotLocatedError if no hunk contains a run of lines matching + `existing_code`. + """ + target_lines = [ + _normalize_line(line) for line in existing_code.splitlines() if line.strip() + ] + if not target_lines: + raise CommentNotLocatedError("existing_code is empty") + + for new_side in (True, False): + for hunk in file: + match = _match_consecutive(_side_lines(hunk, new_side), target_lines) + if match is None: + continue + + line_start, line_end = match + has_added_lines = any(line.is_added for line in hunk) + has_deleted_lines = any(line.is_removed for line in hunk) + if has_added_lines and has_deleted_lines: + hunk_start, hunk_end = find_mixed_lines_range(hunk) + elif has_added_lines: + hunk_start, hunk_end = find_added_lines_range(hunk) + else: + hunk_start, hunk_end = find_removed_lines_range(hunk) + + return { + "line_start": line_start, + "line_end": line_end, + "hunk_start_line": hunk_start, + "hunk_end_line": hunk_end, + "has_added_lines": new_side, + } + + raise CommentNotLocatedError( + f"Could not find existing_code in the patch: {existing_code!r}" + ) + + +def find_line_text(file: PatchedFile, line_number: int) -> str: + """Return the content of the line numbered `line_number` in `file`. + + Checks new-file numbering (context + added lines) first, then old-file + numbering (context + removed lines). Used to backfill `existing_code` for + few-shot examples sourced from historical (file, line_number) comments. + """ + for hunk in file: + for line in hunk: + if line.target_line_no == line_number and not line.is_removed: + return line.value.rstrip("\n") + for hunk in file: + for line in hunk: + if line.source_line_no == line_number and not line.is_added: + return line.value.rstrip("\n") + raise CommentNotLocatedError(f"Line {line_number} not found in {file.path}") def find_added_lines_range(hunk: Hunk): @@ -91,26 +155,50 @@ def get_first_line(_hunk: Hunk, default: int | None = None): def get_hunk_with_associated_lines(hunk): - hunk_with_lines = "" + lines = [] for line in hunk: + content = line.value.rstrip("\n") if line.is_added: - hunk_with_lines += f"{line.target_line_no} + {line.value}" + lines.append(f"{line.target_line_no} + {content}") elif line.is_removed: - hunk_with_lines += f"{line.source_line_no} - {line.value}" + lines.append(f"{line.source_line_no} - {content}") elif line.is_context: - hunk_with_lines += f"{line.target_line_no} {line.value}" + lines.append(f"{line.target_line_no} {content}") - return hunk_with_lines + return "\n".join(lines) def format_patch_set(patch_set): - output = "" + """Render a PatchSet as a unified diff, with an added line-number column. + + The `---`/`+++`/`@@` headers match the unified diff format models are + trained on. The added line-number column isn't part of standard diff + syntax, but gives the model an easy way to double-check counting; comment + placement itself is resolved from quoted `existing_code`, not this number. + """ + output = [] for patch in patch_set: + old_path = ( + patch.source_file + if patch.source_file != "/dev/null" + else f"a/{patch.path}" + ) + new_path = ( + patch.target_file + if patch.target_file != "/dev/null" + else f"b/{patch.path}" + ) + output.append(f"diff --git {old_path} {new_path}") + output.append(f"--- {patch.source_file}") + output.append(f"+++ {patch.target_file}") for hunk in patch: - output += f"Filename: {patch.target_file}\n" - output += f"{get_hunk_with_associated_lines(hunk)}\n" + output.append( + f"@@ -{hunk.source_start},{hunk.source_length} " + f"+{hunk.target_start},{hunk.target_length} @@" + ) + output.append(get_hunk_with_associated_lines(hunk)) - return output + return "\n".join(output) + "\n" def get_associated_file_to_function(function_name, patch): @@ -352,7 +440,10 @@ def convert_generated_comments_to_inline( patch: The PatchSet to validate file paths against. Yields: - InlineComment objects with proper scope information. + InlineComment objects with proper scope information. Comments whose + file or existing_code can't be resolved against the patch are + skipped rather than aborting the whole batch. An unresolved + existing_code is logged at error level (visible in Sentry). """ patched_files_map = { patched_file.target_file: patched_file for patched_file in patch @@ -367,28 +458,35 @@ def convert_generated_comments_to_inline( patched_file = patched_files_map.get(file_path) if patched_file is None: - raise FileNotInPatchError( - f"The file `{file_path}` is not part of the patch: {list(patched_files_map)}" + logger.warning( + "Dropping comment: file `%s` is not part of the patch: %s", + file_path, + list(patched_files_map), ) + continue - line_number = comment.code_line - if not isinstance(line_number, int): - raise ModelResultError("Line number must be an integer") - - scope = find_comment_scope(patched_file, line_number) + try: + location = find_comment_location(patched_file, comment.existing_code) + except CommentNotLocatedError: + logger.error( + "Dropping comment: could not locate existing_code in `%s`: %r", + file_path, + comment.existing_code, + ) + continue yield InlineComment( filename=( patched_file.target_file[2:] - if scope["has_added_lines"] + if location["has_added_lines"] else patched_file.source_file[2:] ), - start_line=line_number, - end_line=line_number, - hunk_start_line=scope["line_start"], - hunk_end_line=scope["line_end"], + start_line=location["line_start"], + end_line=location["line_end"], + hunk_start_line=location["hunk_start_line"], + hunk_end_line=location["hunk_end_line"], content=comment.comment, - on_removed_code=not scope["has_added_lines"], + on_removed_code=not location["has_added_lines"], explanation=comment.explanation, order=comment.order, ) diff --git a/bugbug/tools/core/exceptions.py b/bugbug/tools/core/exceptions.py index e2dd3133f0..3c666615e6 100644 --- a/bugbug/tools/core/exceptions.py +++ b/bugbug/tools/core/exceptions.py @@ -10,12 +10,8 @@ class ModelResultError(Exception): """Occurs when the model returns an unexpected result.""" -class FileNotInPatchError(ModelResultError): - """Occurs when the file in the model result is not part of the patch.""" - - -class HunkNotInPatchError(ModelResultError): - """Occurs when the hunk in the model result is not part of the patch.""" +class CommentNotLocatedError(ModelResultError): + """Occurs when a comment's quoted code can't be matched against the patch.""" class RunawayGenerationError(ModelResultError): diff --git a/tests/test_code_review.py b/tests/test_code_review.py index 8a533766da..b4e80526fa 100644 --- a/tests/test_code_review.py +++ b/tests/test_code_review.py @@ -10,7 +10,7 @@ import pytest from unidiff import PatchSet -from bugbug.tools.code_review import data_types, langchain_tools, review_context +from bugbug.tools.code_review import data_types, langchain_tools, review_context, utils from bugbug.tools.code_review.data_types import ( ExternalContent, GeneratedReviewComment, @@ -44,7 +44,8 @@ from bugbug.tools.code_review.review_context_schema import ( main as validate_review_context_main, ) -from bugbug.tools.code_review.utils import find_comment_scope +from bugbug.tools.code_review.utils import find_comment_location +from bugbug.tools.core.exceptions import CommentNotLocatedError from bugbug.tools.core.platforms.patch_apply import ( apply_patched_file, get_file_after_stack, @@ -282,59 +283,113 @@ def patch_set(self): # --------------------------------------------------------------------------- -# find_comment_scope +# find_comment_location # --------------------------------------------------------------------------- -def test_find_comment_scope(): - test_data = { - (233024, 964198): { - "browser/components/newtab/test/browser/browser.toml": { - 79: { - "line_start": 78, - "line_end": 79, - "has_added_lines": False, - } - }, - "browser/components/asrouter/tests/browser/browser.toml": { - 63: { - "line_start": 60, - "line_end": 74, - "has_added_lines": True, - }, - }, - }, - (240754, 995999): { - "dom/canvas/WebGLShaderValidator.cpp": { - 39: { - "line_start": 37, - "line_end": 42, - "has_added_lines": True, - }, - 46: { - "line_start": 37, - "line_end": 42, - "has_added_lines": True, - }, - } - }, +def _patched_file(raw_diff): + return PatchSet.from_string(raw_diff)[0] + + +def test_find_comment_location_matches_added_line(): + file = _patched_file( + "--- a/f.txt\n+++ b/f.txt\n@@ -1,3 +1,3 @@\n a\n-b\n+B\n c\n" + ) + location = find_comment_location(file, "B") + assert location == { + "line_start": 2, + "line_end": 2, + "hunk_start_line": 1, + "hunk_end_line": 2, + "has_added_lines": True, } - for (revision_id, diff_id), patch_files in test_data.items(): - with open(os.path.join(FIXTURES_DIR, f"D{revision_id}-{diff_id}.diff")) as f: - raw_diff = f.read() - patch_set = PatchSet.from_string(raw_diff) +def test_find_comment_location_matches_multi_line_snippet(): + file = _patched_file( + "--- a/f.txt\n+++ b/f.txt\n@@ -1,2 +1,5 @@\n a\n+x\n+y\n+z\n b\n" + ) + location = find_comment_location(file, "x\ny\nz") + assert location["line_start"] == 2 + assert location["line_end"] == 4 + assert location["has_added_lines"] is True - for file_name, target_hunks in patch_files.items(): - patched_file = next( - patched_file - for patched_file in patch_set - if patched_file.path == file_name - ) - for line_number, expected_scope in target_hunks.items(): - assert find_comment_scope(patched_file, line_number) == expected_scope +def test_find_comment_location_strips_diff_marker_and_whitespace(): + file = _patched_file( + "--- a/f.txt\n+++ b/f.txt\n@@ -1,2 +1,2 @@\n a\n-b\n+B\n" + ) + # A leading '+' and extra whitespace shouldn't prevent a match. + location = find_comment_location(file, " + B \n") + assert location["line_start"] == 2 + assert location["has_added_lines"] is True + + +def test_find_comment_location_matches_removed_line(): + file = _patched_file( + "--- a/f.txt\n+++ b/f.txt\n@@ -1,3 +1,2 @@\n a\n-b\n c\n" + ) + location = find_comment_location(file, "b") + assert location["line_start"] == 2 + assert location["has_added_lines"] is False + + +def test_find_comment_location_not_found_raises(): + file = _patched_file( + "--- a/f.txt\n+++ b/f.txt\n@@ -1,3 +1,3 @@\n a\n-b\n+B\n c\n" + ) + with pytest.raises(CommentNotLocatedError): + find_comment_location(file, "this text does not appear anywhere") + + +def test_find_comment_location_empty_snippet_raises(): + file = _patched_file( + "--- a/f.txt\n+++ b/f.txt\n@@ -1,3 +1,3 @@\n a\n-b\n+B\n c\n" + ) + with pytest.raises(CommentNotLocatedError): + find_comment_location(file, " \n ") + + +def test_find_comment_location_real_patch_mixed_hunk(): + # D240754-995999.diff: a hunk with both added and removed lines. Match on + # the real, verbatim added-line text rather than a hardcoded line number. + with open( + os.path.join(FIXTURES_DIR, "D240754-995999.diff"), encoding="utf-8" + ) as f: + raw_diff = f.read() + + patch_set = PatchSet.from_string(raw_diff) + patched_file = next( + patched_file + for patched_file in patch_set + if patched_file.path == "dom/canvas/WebGLShaderValidator.cpp" + ) + + location = find_comment_location(patched_file, "if (kIsMacOS) {") + assert location["has_added_lines"] is True + assert location["hunk_start_line"] == 37 + assert location["hunk_end_line"] == 42 + + +def test_convert_generated_comments_logs_error_when_unlocated(caplog): + patch_set = PatchSet.from_string( + "--- a/f.txt\n+++ b/f.txt\n@@ -1,3 +1,3 @@\n a\n-b\n+B\n c\n" + ) + comment = GeneratedReviewComment( + file="b/f.txt", + existing_code="this text does not appear anywhere", + comment="bad", + explanation="x", + order=1, + ) + with caplog.at_level(logging.ERROR, logger=utils.logger.name): + result = list(utils.convert_generated_comments_to_inline([comment], patch_set)) + + assert result == [] + assert any( + record.levelno == logging.ERROR and "Dropping comment" in record.message + for record in caplog.records + ) def _mock_client_returning(text: str) -> MagicMock: @@ -1289,7 +1344,7 @@ def test_assess_patch_scope_returns_at_most_one_comment(): comments = [ GeneratedReviewComment( file="b/f.txt", - code_line=1, + existing_code="a", comment=f"Split this patch {i}", explanation="bundles unrelated changes", order=1, @@ -1319,14 +1374,14 @@ def test_run_appends_scope_suggestion_last(): regular = GeneratedReviewComment( file="b/f.txt", - code_line=1, + existing_code="a", comment="Fix the bug", explanation="off-by-one", order=1, ) scope = GeneratedReviewComment( file="b/f.txt", - code_line=2, + existing_code="b", comment="Split this patch into smaller pieces", explanation="bundles unrelated changes", order=1,