mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-01 23:13:15 +00:00
1d4f1aed33
* fix: resolve reviewer head_sha from thread metadata, not frozen run config A push that lands while a reviewer run is in flight is delivered as a queued message into that run. The run's configurable is frozen at creation, so its head_sha still names the commit the run was created for — not the commit just pushed. publish_review then anchored the GitHub review to the stale commit and regressed last_reviewed_sha to it, and add_finding/update_finding stamped findings with the stale SHA. Persist the current head in thread metadata at every reviewer dispatch (both the ready-for-review and push paths, before they branch to create a run or queue a message), and add resolve_review_head_sha() which prefers the metadata head over the run config. Wire it into publish_review (review commit_id + last_reviewed_sha), add_finding (first_seen_sha) and update_finding (last_confirmed_sha). Falls back to the run config when metadata carries no head (first review, eval, tests). * fix: persist head_sha in manual review dispatch (trigger_pr_review_from_ref) resolve_review_head_sha prefers metadata[head_sha] over the run config, and the push/ready dispatchers write it — but trigger_pr_review_from_ref (Slack/GitHub @open-swe review, request_pr_review tool) created a run with a freshly-fetched config head while leaving metadata's head stale from a prior dispatch. A manual re-review at a newer commit would then resolve to the old head and publish/advance findings against it. Persist head_sha in that dispatch's metadata write too, so every run-creating reviewer dispatch keeps metadata in sync with the head its run targets. Caught by the Open SWE reviewer on this PR.
222 lines
7.9 KiB
Python
222 lines
7.9 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,
|
|
resolve_review_head_sha,
|
|
set_reviewer_thread_metadata,
|
|
update_finding_fields,
|
|
)
|
|
|
|
|
|
def _f(**overrides: Any) -> Finding:
|
|
base = new_finding(
|
|
severity="high",
|
|
confidence="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_id"] is None
|
|
assert finding["github_review_comment_id"] is None
|
|
assert finding["github_review_comment_ids"] == []
|
|
assert finding["github_review_thread_id"] is None
|
|
assert finding["github_review_thread_ids"] == []
|
|
assert finding["github_review_run_id"] is None
|
|
assert finding["github_thread_resolved"] is False
|
|
assert finding["github_resolved_thread_ids"] == []
|
|
assert finding["last_human_reply_at"] is None
|
|
assert finding["resolution_note"] is None
|
|
assert finding["suggestion"] is None
|
|
|
|
|
|
def test_severity_order_monotonic() -> None:
|
|
assert (
|
|
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"),
|
|
]
|
|
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
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_set_reviewer_thread_metadata_persists_head_sha() -> None:
|
|
fake_client = AsyncMock()
|
|
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
|
|
await set_reviewer_thread_metadata("tid", head_sha="newhead")
|
|
metadata = fake_client.threads.update.await_args.kwargs["metadata"]
|
|
assert metadata["head_sha"] == "newhead"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_review_head_sha_prefers_metadata_over_config() -> None:
|
|
"""A mid-run push records the live head in thread metadata; it must win over
|
|
the stale head frozen in the run's config."""
|
|
fake_client = AsyncMock()
|
|
fake_client.threads.get.return_value = {"metadata": {"head_sha": "metahead"}}
|
|
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
|
|
head = await resolve_review_head_sha("tid", {"head_sha": "confighead"})
|
|
assert head == "metahead"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_review_head_sha_falls_back_to_config_when_metadata_empty() -> None:
|
|
fake_client = AsyncMock()
|
|
fake_client.threads.get.return_value = {"metadata": {}}
|
|
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
|
|
head = await resolve_review_head_sha("tid", {"head_sha": "confighead"})
|
|
assert head == "confighead"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_review_head_sha_falls_back_without_thread_id() -> None:
|
|
fake_client = AsyncMock()
|
|
with patch("agent.reviewer_findings.get_client", return_value=fake_client):
|
|
head = await resolve_review_head_sha("", {"head_sha": "confighead"})
|
|
assert head == "confighead"
|
|
fake_client.threads.get.assert_not_called()
|