mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 12:43:16 +00:00
378b95266e
* feat: implement reviewer findings, publish_review, and watch mode
Build out the reviewer agent end-to-end against the design in
REVIEWER_DESIGN.md:
- Findings as first-class state on the reviewer thread metadata
(`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line
ranges, suggestion text for ```suggestion blocks, github_review_comment_id
for cross-run reconciliation, diff_hunk for UI rendering. Thread-level
metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a
future frontend can list reviewer threads via the langgraph SDK.
- Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff,
compute_diff_line_set for in-diff validation, extract_diff_hunk for
caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA
diffs against the prepped repo.
- Tools: `add_finding` (validates against the diff line set so out-of-diff
ranges fail at creation, not at GitHub-publish), `update_finding`,
`list_findings`, `publish_review`. The reviewer agent's tool list is
swapped from `[]` (direct shell `gh api` calls) to these four.
- Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`):
one POST /reviews call with body + inline comments + ```suggestion blocks,
per-comment IDs stored back on findings, GraphQL `resolveReviewThread`
fired for findings transitioning open->resolved on a re-review.
- Reviewer graph: deterministic clone-or-fetch + checkout in the factory
before the agent's first model call (warm- and cold-path symmetric);
computed diff and in-diff line set passed via runnable config; system
prompt rewritten for the single-evolving-findings model, severity ladder,
in-diff-only discipline, and watch-mode reconciliation flow.
- Watch mode in webapp.py: `push` event + `pull_request` closed/reopened
added to supported events. New `process_github_push_event` resolves the
open PR for the pushed branch, gates on the reviewer thread's `watch`
flag, builds a re-review configurable, and triggers a run on the same
canonical thread. `process_github_pr_close` toggles watch on
closed/reopened. `set_reviewer_thread_metadata` is called on first
review to install `kind=reviewer` + PR identity + watch=True.
- Eval harness: target.py now extracts `add_finding` calls (mapped to the
legacy {file, line, body, severity} shape the judge expects) and passes
the right configurable so the prep step has base/head SHAs.
- Tests: new unit suites for findings helpers, diff parsing, finding tools,
publish rendering + GraphQL resolve, and watch-mode webhook handlers
(push triggers re-review only when watching, idempotent on unchanged
head SHA, PR close disables watch). Updated existing reviewer-webhook
tests to mock `set_reviewer_thread_metadata`.
- REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context
into REVIEWER_DESIGN.md.
* fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL
Address PR #1253 review findings:
- compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag
(`option no-prefix takes no value` — every prep run was failing
silently and the agent saw an empty diff).
- compute_diff_in_sandbox grew a `merge_base` flag. First-review path
now uses three-dot `base...head` (the merge-base diff GitHub renders
on Files-changed) so we don't pick up changes that landed on the base
branch after the PR diverged. Re-review delta keeps two-dot
`last_reviewed_sha..head` since that's exactly the new commits.
- publish_review skips findings that already carry
`github_review_comment_id`. Without this, watched re-reviews
re-posted every previously surfaced finding, and only the most-recent
duplicate's id would later resolve when the issue got addressed.
- fetch_review_comments URL now includes `{pull_number}` —
`/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments`
is the canonical endpoint; the old form 404s, so comment ids were
never stored and watch-mode resolution couldn't run.
Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix`
flag in the executed command, and that publish_review does not re-post
findings whose `github_review_comment_id` is set.
* fix(reviewer): default publish cap from 15 to 4
A clean PR with one critical issue padded out by three lower-severity
findings is fine; fifteen is review spam. The agent can override per
call when a PR genuinely warrants more.
112 lines
3.4 KiB
Python
112 lines
3.4 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"] == {10, 11, 12, 13}
|
|
assert line_set["bar.py"] == {1, 2, 3, 51, 52, 53, 54}
|
|
|
|
|
|
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_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
|