open-swe/tests/test_reviewer_findings.py
Johannes du Plessis 378b95266e
feat: implement reviewer findings, publish_review, and watch mode (#1253)
* 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.
2026-05-07 14:48:43 -07:00

175 lines
5.8 KiB
Python

"""Unit tests for the Finding schema + thread-metadata helpers."""
from __future__ import annotations
from typing import Any
from unittest.mock import AsyncMock, patch
import pytest
from agent.reviewer_findings import (
SEVERITY_ORDER,
Finding,
append_finding,
filter_findings_for_publish,
list_findings,
new_finding,
new_finding_id,
replace_findings,
set_reviewer_thread_metadata,
update_finding_fields,
)
def _f(**overrides: Any) -> Finding:
base = new_finding(
severity="high",
category="correctness",
file="foo.py",
start_line=10,
end_line=10,
description="boom",
sha="abc123",
)
base.update(overrides) # type: ignore[arg-type]
return base
def test_new_finding_id_format() -> None:
fid = new_finding_id()
assert fid.startswith("f_")
assert len(fid) == len("f_") + 10
def test_new_finding_defaults() -> None:
finding = _f()
assert finding["status"] == "open"
assert finding["side"] == "RIGHT"
assert finding["first_seen_sha"] == "abc123"
assert finding["last_confirmed_sha"] == "abc123"
assert finding["github_review_comment_id"] is None
assert finding["suggestion"] is None
def test_severity_order_monotonic() -> None:
assert (
SEVERITY_ORDER["informational"]
< SEVERITY_ORDER["low"]
< SEVERITY_ORDER["medium"]
< SEVERITY_ORDER["high"]
< SEVERITY_ORDER["critical"]
)
def test_filter_findings_for_publish_drops_below_threshold_and_resolved() -> None:
findings = [
_f(id="f_a", severity="high", file="a.py", start_line=1, end_line=1),
_f(id="f_b", severity="low", file="b.py"),
_f(id="f_c", severity="critical", file="c.py", start_line=2, end_line=2),
_f(id="f_d", severity="high", file="d.py", status="resolved"),
_f(id="f_e", severity="informational", file="e.py"),
]
surfaced = filter_findings_for_publish(findings, severity_threshold="medium", cap=10)
assert [f["id"] for f in surfaced] == ["f_c", "f_a"]
def test_filter_findings_for_publish_caps_results() -> None:
findings = [_f(id=f"f_{i}", severity="high", file=f"f{i}.py") for i in range(20)]
surfaced = filter_findings_for_publish(findings, severity_threshold="medium", cap=5)
assert len(surfaced) == 5
@pytest.mark.asyncio
async def test_list_findings_returns_empty_on_missing_metadata() -> None:
fake_client = AsyncMock()
fake_client.threads.get.return_value = {"metadata": {}}
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
findings = await list_findings("tid")
assert findings == []
@pytest.mark.asyncio
async def test_list_findings_coerces_bad_entries() -> None:
fake_client = AsyncMock()
fake_client.threads.get.return_value = {
"metadata": {
"findings": [
{"id": "f_ok", "severity": "high", "file": "x.py"},
{"missing_id": True},
"not-a-dict",
]
}
}
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
findings = await list_findings("tid")
assert [f["id"] for f in findings] == ["f_ok"]
@pytest.mark.asyncio
async def test_replace_findings_calls_threads_update() -> None:
fake_client = AsyncMock()
findings = [_f(id="f_x")]
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
await replace_findings("tid", findings)
fake_client.threads.update.assert_awaited_once_with(
thread_id="tid", metadata={"findings": findings}
)
@pytest.mark.asyncio
async def test_append_finding_appends_to_existing_list() -> None:
existing = _f(id="f_a")
new = _f(id="f_b")
fake_client = AsyncMock()
fake_client.threads.get.return_value = {"metadata": {"findings": [existing]}}
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
result = await append_finding("tid", new)
assert result["id"] == "f_b"
args = fake_client.threads.update.await_args
persisted = args.kwargs["metadata"]["findings"]
assert [f["id"] for f in persisted] == ["f_a", "f_b"]
@pytest.mark.asyncio
async def test_update_finding_fields_mutates_only_target() -> None:
a = _f(id="f_a", description="orig-a")
b = _f(id="f_b", description="orig-b")
fake_client = AsyncMock()
fake_client.threads.get.return_value = {"metadata": {"findings": [a, b]}}
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
updated = await update_finding_fields("tid", "f_b", {"status": "resolved"})
assert updated is not None
assert updated["status"] == "resolved"
persisted = fake_client.threads.update.await_args.kwargs["metadata"]["findings"]
by_id = {f["id"]: f for f in persisted}
assert by_id["f_a"]["status"] == "open"
assert by_id["f_b"]["status"] == "resolved"
@pytest.mark.asyncio
async def test_update_finding_fields_returns_none_for_unknown_id() -> None:
fake_client = AsyncMock()
fake_client.threads.get.return_value = {"metadata": {"findings": [_f(id="f_a")]}}
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
result = await update_finding_fields("tid", "f_missing", {"status": "resolved"})
assert result is None
fake_client.threads.update.assert_not_called()
@pytest.mark.asyncio
async def test_set_reviewer_thread_metadata_includes_kind() -> None:
fake_client = AsyncMock()
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
await set_reviewer_thread_metadata("tid", watch=True, last_reviewed_sha="sha")
metadata = fake_client.threads.update.await_args.kwargs["metadata"]
assert metadata["kind"] == "reviewer"
assert metadata["watch"] is True
assert metadata["last_reviewed_sha"] == "sha"
assert "pr" not in metadata
assert "findings" not in metadata