From b2a6fe2b3874d5e5a31b5e56f620f6103c0606f8 Mon Sep 17 00:00:00 2001 From: Paul Adenot Date: Mon, 3 Aug 2026 14:27:43 +0200 Subject: [PATCH 1/3] code_review: render patch diffs with real git diff headers format_patch_set() repeated "Filename: X" before every hunk and had no --- / +++ / @@ headers, so the diff shown to the model didn't pattern-match a real unified diff. Add diff --git/---/+++/@@ headers (printed once per file) while keeping the per-line number column that lets the model anchor comments without counting from the hunk header. --- bugbug/tools/code_review/utils.py | 42 ++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/bugbug/tools/code_review/utils.py b/bugbug/tools/code_review/utils.py index f242111b87..ecec5606b5 100644 --- a/bugbug/tools/code_review/utils.py +++ b/bugbug/tools/code_review/utils.py @@ -91,26 +91,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 line-number column (absolute, per source/target file) lets + the model anchor comments to a `code_line` without counting from the hunk + header. + """ + 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): From 3744d2d57e95663f3aa96f56da20211f2da441d6 Mon Sep 17 00:00:00 2001 From: Paul Adenot Date: Mon, 3 Aug 2026 14:45:11 +0200 Subject: [PATCH 2/3] code_review: locate comments by quoted code instead of a line number Trusting the model to output an absolute code_line requires it to count from a hunk header; miscounting either silently mislocates the comment (find_comment_scope only checked the line fell inside some hunk, not that it was the right one) or, if the number falls outside every hunk, raises and kills every other comment in the batch too. Adopt the approach used by alibaba/open-code-review: replace code_line with existing_code, a verbatim quote of the line(s) the comment is about. find_comment_location() deterministically matches that quote against the patch's hunks (new side, then old side) to derive the real line range, instead of trusting model arithmetic. Comments that can't be matched are now dropped and logged rather than raising, so one bad comment no longer costs every other one. Fixes REVIEWHELPER-API-2C --- bugbug/tools/code_review/__init__.py | 6 +- bugbug/tools/code_review/agent.py | 45 +++++-- bugbug/tools/code_review/data_types.py | 9 +- bugbug/tools/code_review/prompts.py | 3 +- bugbug/tools/code_review/utils.py | 176 ++++++++++++++++++------- bugbug/tools/core/exceptions.py | 8 +- tests/test_code_review.py | 132 ++++++++++++------- 7 files changed, 259 insertions(+), 120 deletions(-) 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 ecec5606b5..5c5748f3a0 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): @@ -108,9 +172,9 @@ def format_patch_set(patch_set): """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 line-number column (absolute, per source/target file) lets - the model anchor comments to a `code_line` without counting from the hunk - header. + 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: @@ -376,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 (and logged) rather than aborting the whole batch — one bad + comment shouldn't cost the reviewee every other comment. """ patched_files_map = { patched_file.target_file: patched_file for patched_file in patch @@ -391,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.warning( + "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..061de7a4b1 100644 --- a/tests/test_code_review.py +++ b/tests/test_code_review.py @@ -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,92 @@ 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 _mock_client_returning(text: str) -> MagicMock: @@ -1289,7 +1323,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 +1353,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, From 9f5405a1b114991503a71326355ec462a02fe88a Mon Sep 17 00:00:00 2001 From: Paul Adenot Date: Mon, 3 Aug 2026 14:49:44 +0200 Subject: [PATCH 3/3] code_review: log unlocated existing_code as an error Dropping a comment because its existing_code doesn't match anywhere in the patch was only a warning, easy to miss. Log it at error level instead so it surfaces in Sentry (already capturing ERROR+ logs in the services that embed this library) rather than only appearing in debug logs no one is watching. --- bugbug/tools/code_review/utils.py | 6 +++--- tests/test_code_review.py | 23 ++++++++++++++++++++++- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/bugbug/tools/code_review/utils.py b/bugbug/tools/code_review/utils.py index 5c5748f3a0..8ff9e1a1b3 100644 --- a/bugbug/tools/code_review/utils.py +++ b/bugbug/tools/code_review/utils.py @@ -442,8 +442,8 @@ def convert_generated_comments_to_inline( Yields: InlineComment objects with proper scope information. Comments whose file or existing_code can't be resolved against the patch are - skipped (and logged) rather than aborting the whole batch — one bad - comment shouldn't cost the reviewee every other comment. + 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 @@ -468,7 +468,7 @@ def convert_generated_comments_to_inline( try: location = find_comment_location(patched_file, comment.existing_code) except CommentNotLocatedError: - logger.warning( + logger.error( "Dropping comment: could not locate existing_code in `%s`: %r", file_path, comment.existing_code, diff --git a/tests/test_code_review.py b/tests/test_code_review.py index 061de7a4b1..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, @@ -371,6 +371,27 @@ def test_find_comment_location_real_patch_mixed_hunk(): 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: response = MagicMock() response.text = text