adam/open-swe
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/open-swe.git synced 2026-09-30 10:23:14 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: reviewer finding thread resolution state (#1348)

Browse source 
* fix: keep reviewer findings open until threads resolve

* fix: allow hidden reviewer findings to resolve locally
This commit is contained in:
Johannes du Plessis 2026-05-28 09:16:30 -07:00 • committed by GitHub
parent 197d339df4
commit 68069ea058
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 188 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
agent/reviewer_reconcile.py
View file

@ -16,7 +16,7 @@ ReviewThreadMatch = tuple[ReviewThread, int | None]
def _is_open_swe_bot_comment(comment: ReviewThread) -> bool:
return comment.get("author") == "open-swe[bot]"
return comment.get("author") in {"open-swe", "open-swe[bot]"}
def _int_list(value: Any) -> list[int]:
@ -52,7 +52,7 @@ def _human_replies_after_bot_comment(
if not seen_bot_comment:
continue
author = comment.get("author")
if author == "open-swe[bot]":
if author in {"open-swe", "open-swe[bot]"}:
continue
replies.append(comment)
return replies

68
agent/tools/update_finding.py
View file

@ -9,12 +9,35 @@ from langgraph.config import get_config
from ..reviewer_findings import (
MAX_SUGGESTION_LINES,
Finding,
clip_suggestion,
get_thread_id_from_runtime,
list_findings,
update_finding_fields,
)
def _is_non_empty_str(value: Any) -> bool:
return isinstance(value, str) and bool(value)
def _has_published_github_surface(finding: Finding) -> bool:
surface = finding.get("surface")
if isinstance(surface, dict) and (
isinstance(surface.get("github_review_comment_id"), int)
or _is_non_empty_str(surface.get("github_review_thread_id"))
):
return True
comment_ids = finding.get("github_review_comment_ids")
thread_ids = finding.get("github_review_thread_ids")
return (
isinstance(finding.get("github_review_comment_id"), int)
or _is_non_empty_str(finding.get("github_review_thread_id"))
or (isinstance(comment_ids, list) and any(isinstance(item, int) for item in comment_ids))
or (isinstance(thread_ids, list) and any(_is_non_empty_str(item) for item in thread_ids))
)
def update_finding(
finding_id: str,
status: str | None = None,
@ -96,10 +119,11 @@ def update_finding(
return {"success": False, "error": "No fields provided to update"}
thread_id = get_thread_id_from_runtime()
updated = asyncio.run(update_finding_fields(thread_id, finding_id, updates))
if updated is None:
findings = asyncio.run(list_findings(thread_id))
finding = next((item for item in findings if item.get("id") == finding_id), None)
if finding is None:
return {"success": False, "error": f"No finding found with id {finding_id}"}
result: dict[str, Any] = {"success": True, "finding": updated}
repo_config = configurable.get("repo") if isinstance(configurable, dict) else None
pr_number = configurable.get("pr_number") if isinstance(configurable, dict) else None
can_resolve_github_thread = (
@ -108,13 +132,41 @@ def update_finding(
and bool(repo_config.get("name"))
and isinstance(pr_number, int)
)
if status in {"resolved", "dismissed"} and can_resolve_github_thread:
if (
status in {"resolved", "dismissed"}
and can_resolve_github_thread
and _has_published_github_surface(finding)
):
from .resolve_finding_thread import resolve_finding_thread
resolve_result = resolve_finding_thread(finding_id, status=status)
result["github_resolution"] = resolve_result
if resolve_result.get("success"):
result["finding"] = resolve_result.get("finding", updated)
resolve_result = resolve_finding_thread(finding_id, status=status, note=note)
if not resolve_result.get("success"):
return {
"success": False,
"error": "GitHub review thread resolution failed; finding was left open.",
"github_resolution": resolve_result,
}
updates.pop("status", None)
updates.pop("last_update_note", None)
if not updates:
result: dict[str, Any] = {
"success": True,
"finding": resolve_result.get("finding"),
"github_resolution": resolve_result,
}
if suggestion_dropped:
result["suggestion_dropped"] = True
result["warning"] = (
f"Suggestion exceeded the {MAX_SUGGESTION_LINES}-line cap and was "
"rejected — the finding's prior `suggestion` was left unchanged. "
"Only include `suggestion` for small, obvious fixes."
)
return result
updated = asyncio.run(update_finding_fields(thread_id, finding_id, updates))
if updated is None:
return {"success": False, "error": f"No finding found with id {finding_id}"}
result = {"success": True, "finding": updated}
if suggestion_dropped:
result["suggestion_dropped"] = True
result["warning"] = (

43
tests/test_reviewer_reconcile.py
View file

@ -85,6 +85,49 @@ async def test_reconcile_backfills_comment_and_thread_ids_from_bot_marker() -> N
replace.assert_awaited_once()
@pytest.mark.asyncio
async def test_reconcile_backfills_marker_from_graphql_app_login() -> None:
findings = [
{
"id": "f1",
"status": "open",
"github_review_comment_id": None,
"github_review_thread_id": None,
}
]
replace = AsyncMock()
with (
patch("agent.reviewer_reconcile.list_findings", AsyncMock(return_value=findings)),
patch("agent.reviewer_reconcile.replace_findings", replace),
):
result = await reconcile_findings_with_review_threads(
"tid",
[
{
"id": "THREAD_1",
"is_resolved": False,
"is_outdated": False,
"comments": [
{
"id": 11,
"author": "open-swe",
"body": (
'<!-- open-swe-review-comment {"id":"f1",'
'"file_path":"a.py","start_line":1,'
'"end_line":1,"side":"RIGHT"} -->\n\nbug'
),
}
],
}
],
)
assert result[0]["github_review_comment_id"] == 11
assert result[0]["github_review_thread_id"] == "THREAD_1"
replace.assert_awaited_once()
@pytest.mark.asyncio
async def test_reconcile_duplicate_markers_require_all_threads_terminal() -> None:
findings = [

87
tests/test_reviewer_tools.py
View file

@ -27,6 +27,12 @@ def _config(**configurable_overrides: Any) -> dict[str, Any]:
return base
def _existing_finding(**overrides: Any) -> dict[str, Any]:
finding: dict[str, Any] = {"id": "f_a", "status": "open"}
finding.update(overrides)
return finding
def test_add_finding_rejects_invalid_severity() -> None:
with patch("agent.tools.add_finding.get_config", return_value=_config()):
result = add_finding(
@ -337,6 +343,10 @@ def test_update_finding_rejects_long_suggestion_without_clobbering() -> None:
with (
patch("agent.tools.update_finding.get_config", return_value=_config()),
patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"),
patch(
"agent.tools.update_finding.list_findings",
AsyncMock(return_value=[_existing_finding()]),
),
patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update),
):
result = update_finding(
@ -375,6 +385,10 @@ def test_update_finding_empty_string_clears_suggestion() -> None:
with (
patch("agent.tools.update_finding.get_config", return_value=_config()),
patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"),
patch(
"agent.tools.update_finding.list_findings",
AsyncMock(return_value=[_existing_finding()]),
),
patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update),
):
result = update_finding(finding_id="f_a", suggestion="")
@ -393,6 +407,10 @@ def test_update_finding_passes_through_fields() -> None:
with (
patch("agent.tools.update_finding.get_config", return_value=_config()),
patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"),
patch(
"agent.tools.update_finding.list_findings",
AsyncMock(return_value=[_existing_finding()]),
),
patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update),
):
result = update_finding(
@ -409,16 +427,17 @@ def test_update_finding_passes_through_fields() -> None:
def test_update_finding_resolves_github_thread_when_pr_context_available() -> None:
async def fake_update(thread_id: str, finding_id: str, updates: Any) -> Any:
return {"id": finding_id, **updates}
cfg = _config(repo={"owner": "o", "name": "r"}, pr_number=7)
with (
patch("agent.tools.update_finding.get_config", return_value=cfg),
patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"),
patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update),
patch(
"agent.tools.update_finding.list_findings",
AsyncMock(return_value=[_existing_finding(github_review_thread_id="THREAD_1")]),
),
patch("agent.tools.resolve_finding_thread.get_config", return_value=cfg),
patch("agent.tools.resolve_finding_thread.get_github_token", return_value="token"),
patch("agent.tools.update_finding.update_finding_fields", AsyncMock()) as update,
patch(
"agent.tools.resolve_finding_thread._resolve_finding_thread_async",
new_callable=AsyncMock,
@ -435,6 +454,66 @@ def test_update_finding_resolves_github_thread_when_pr_context_available() -> No
assert result["github_resolution"]["success"] is True
assert result["finding"]["github_thread_resolved"] is True
resolve_async.assert_awaited_once()
update.assert_not_awaited()
def test_update_finding_leaves_open_when_github_resolution_fails() -> None:
cfg = _config(repo={"owner": "o", "name": "r"}, pr_number=7)
with (
patch("agent.tools.update_finding.get_config", return_value=cfg),
patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"),
patch(
"agent.tools.update_finding.list_findings",
AsyncMock(return_value=[_existing_finding(github_review_thread_id="THREAD_1")]),
),
patch("agent.tools.resolve_finding_thread.get_config", return_value=cfg),
patch("agent.tools.resolve_finding_thread.get_github_token", return_value="token"),
patch("agent.tools.update_finding.update_finding_fields", AsyncMock()) as update,
patch(
"agent.tools.resolve_finding_thread._resolve_finding_thread_async",
new_callable=AsyncMock,
return_value={
"success": False,
"error": "Could not resolve GitHub review thread id",
},
) as resolve_async,
):
result = update_finding(finding_id="f_a", status="resolved")
assert result["success"] is False
assert "left open" in result["error"]
assert result["github_resolution"]["error"] == "Could not resolve GitHub review thread id"
resolve_async.assert_awaited_once()
update.assert_not_awaited()
def test_update_finding_resolves_hidden_finding_locally() -> None:
captured: list[Any] = []
async def fake_update(thread_id: str, finding_id: str, updates: Any) -> Any:
captured.append((thread_id, finding_id, updates))
return {"id": finding_id, **updates}
cfg = _config(repo={"owner": "o", "name": "r"}, pr_number=7)
with (
patch("agent.tools.update_finding.get_config", return_value=cfg),
patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"),
patch(
"agent.tools.update_finding.list_findings",
AsyncMock(return_value=[_existing_finding()]),
),
patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update),
patch(
"agent.tools.resolve_finding_thread._resolve_finding_thread_async",
new_callable=AsyncMock,
) as resolve_async,
):
result = update_finding(finding_id="f_a", status="resolved")
assert result["success"] is True
_thread_id, _finding_id, updates = captured[0]
assert updates["status"] == "resolved"
resolve_async.assert_not_awaited()
def test_list_findings_filters_by_status() -> None: