mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 11:33:14 +00:00
fa147ce256
* reviewer: fetch PR diff via GitHub API to re-enable add_finding validation The previous hotfix in reviewer.py set diff_line_set=None because the sandbox-based diff prep was sometimes producing empty diffs. That made every bad anchor a publish-time 422 instead of a creation-time rejection — the agent burned tokens producing unanchorable findings, and we had to add a publish-time retry safety net (#1338) to clean up. Fetch the PR's unified diff via the GitHub REST API at reviewer startup and populate diff_text + diff_line_set so add_finding can reject bad anchors immediately. The API path is reliable and is the same diff GitHub validates against when posting inline review comments. If the fetch fails, fall back to the previous behavior (validation disabled, publish-time retry handles it). Also extract the PR-diff fetch into reviewer_diff.fetch_pr_diff so both reviewer.py and publish_review.py share one implementation instead of two copies. * reviewer: make diff_line_set validation side-aware compute_diff_line_set previously returned only new-side line numbers, so re-enabling add_finding's validation would wrongly reject findings with side=LEFT (deleted-line bugs whose only anchor is an old-side line). Return {file: {"RIGHT": {new_lines}, "LEFT": {old_lines}}} instead, and have is_range_in_diff select the matching side from the finding's recorded side. add_finding and publish_review's retry filter both pass the finding's side through.
133 lines
4.5 KiB
Python
133 lines
4.5 KiB
Python
"""Unit tests for the unified-diff parsing helpers."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
|
|
from agent.reviewer_diff import (
|
|
compute_diff_line_set,
|
|
extract_diff_hunk,
|
|
is_range_in_diff,
|
|
parse_unified_diff,
|
|
)
|
|
|
|
_TWO_FILE_DIFF = """diff --git a/foo.py b/foo.py
|
|
index 1111111..2222222 100644
|
|
--- a/foo.py
|
|
+++ b/foo.py
|
|
@@ -10,3 +10,4 @@ def existing():
|
|
pass
|
|
+ new_line_13 = 1
|
|
+ new_line_14 = 2
|
|
return 1
|
|
diff --git a/bar.py b/bar.py
|
|
index 3333333..4444444 100644
|
|
--- a/bar.py
|
|
+++ b/bar.py
|
|
@@ -1,2 +1,3 @@
|
|
import os
|
|
+import sys
|
|
print(os.getcwd())
|
|
@@ -50,3 +51,4 @@ def other():
|
|
line_a = 1
|
|
+ line_b = 2
|
|
line_c = 3
|
|
"""
|
|
|
|
|
|
def test_parse_unified_diff_extracts_hunks_per_file() -> None:
|
|
files = parse_unified_diff(_TWO_FILE_DIFF)
|
|
assert [fd.file for fd in files] == ["foo.py", "bar.py"]
|
|
assert len(files[0].hunks) == 1
|
|
assert len(files[1].hunks) == 2
|
|
|
|
|
|
def test_compute_diff_line_set_covers_each_hunks_new_lines() -> None:
|
|
line_set = compute_diff_line_set(_TWO_FILE_DIFF)
|
|
assert line_set["foo.py"]["RIGHT"] == {10, 11, 12, 13}
|
|
assert line_set["bar.py"]["RIGHT"] == {1, 2, 3, 51, 52, 53, 54}
|
|
|
|
|
|
def test_compute_diff_line_set_also_covers_old_side_lines() -> None:
|
|
"""LEFT-side findings anchor to deleted/old-side lines; the line set
|
|
must expose those so add_finding doesn't wrongly reject them."""
|
|
line_set = compute_diff_line_set(_TWO_FILE_DIFF)
|
|
assert line_set["foo.py"]["LEFT"] == {10, 11, 12}
|
|
assert line_set["bar.py"]["LEFT"] == {1, 2, 50, 51, 52}
|
|
|
|
|
|
def test_is_range_in_diff_for_inline_and_file_level() -> None:
|
|
line_set = compute_diff_line_set(_TWO_FILE_DIFF)
|
|
assert is_range_in_diff(line_set, "foo.py", 11, 12) is True
|
|
assert is_range_in_diff(line_set, "foo.py", 11, 99) is False
|
|
assert is_range_in_diff(line_set, "missing.py", 1, 1) is False
|
|
assert is_range_in_diff(line_set, "foo.py", None, None) is True
|
|
|
|
|
|
def test_is_range_in_diff_left_side_accepts_old_line_numbers() -> None:
|
|
"""A finding with side=LEFT must validate against the OLD-side line set,
|
|
not the new-side. The new-side hunk for foo.py is +10..+13; the old-side
|
|
is -10..-12. Asserting against the wrong side would falsely reject a
|
|
valid deleted-line finding."""
|
|
line_set = compute_diff_line_set(_TWO_FILE_DIFF)
|
|
assert is_range_in_diff(line_set, "foo.py", 12, 12, side="LEFT") is True
|
|
# And the same line on RIGHT side is also in-diff (it's context).
|
|
assert is_range_in_diff(line_set, "foo.py", 12, 12, side="RIGHT") is True
|
|
# A LEFT anchor on a line that doesn't exist on the old side must be rejected.
|
|
assert is_range_in_diff(line_set, "foo.py", 13, 13, side="LEFT") is False
|
|
|
|
|
|
def test_extract_diff_hunk_returns_overlapping_hunk_body() -> None:
|
|
hunk = extract_diff_hunk(_TWO_FILE_DIFF, "bar.py", 51, 52)
|
|
assert hunk is not None
|
|
assert "@@ -50,3 +51,4 @@" in hunk
|
|
assert "line_b" in hunk
|
|
|
|
|
|
def test_extract_diff_hunk_returns_none_for_unknown_file() -> None:
|
|
assert extract_diff_hunk(_TWO_FILE_DIFF, "unknown.py", 1, 1) is None
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
("start", "end"),
|
|
[(1, 1), (1, 3)],
|
|
)
|
|
def test_extract_diff_hunk_supports_single_line_and_range(start: int, end: int) -> None:
|
|
hunk = extract_diff_hunk(_TWO_FILE_DIFF, "bar.py", start, end)
|
|
assert hunk is not None
|
|
assert "import sys" in hunk
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_compute_diff_in_sandbox_uses_three_dot_for_merge_base() -> None:
|
|
"""First-review path passes merge_base=True so we use base...head, not base..head."""
|
|
from unittest.mock import MagicMock
|
|
|
|
from agent.reviewer_diff import compute_diff_in_sandbox
|
|
|
|
backend = MagicMock()
|
|
backend.execute = MagicMock(return_value="")
|
|
|
|
await compute_diff_in_sandbox(
|
|
backend, work_dir="/w", base_ref="base", head_ref="head", merge_base=True
|
|
)
|
|
cmd = backend.execute.call_args.args[0]
|
|
assert "base...head" in cmd
|
|
assert "base..head" not in cmd.replace("base...head", "")
|
|
assert "--no-prefix" not in cmd # invalid flag must not appear
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_compute_diff_in_sandbox_uses_two_dot_by_default() -> None:
|
|
"""Re-review delta path passes merge_base=False so we use base..head."""
|
|
from unittest.mock import MagicMock
|
|
|
|
from agent.reviewer_diff import compute_diff_in_sandbox
|
|
|
|
backend = MagicMock()
|
|
backend.execute = MagicMock(return_value="")
|
|
|
|
await compute_diff_in_sandbox(backend, work_dir="/w", base_ref="oldsha", head_ref="newsha")
|
|
cmd = backend.execute.call_args.args[0]
|
|
assert "oldsha..newsha" in cmd
|
|
assert "oldsha...newsha" not in cmd
|