diff --git a/agent/reviewer_reconcile.py b/agent/reviewer_reconcile.py index d7decf5b..d9659ace 100644 --- a/agent/reviewer_reconcile.py +++ b/agent/reviewer_reconcile.py @@ -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 diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py index 10f6fd7d..61b3f4f5 100644 --- a/agent/tools/update_finding.py +++ b/agent/tools/update_finding.py @@ -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"] = ( diff --git a/tests/test_reviewer_reconcile.py b/tests/test_reviewer_reconcile.py index 2bfc155d..fe61df4c 100644 --- a/tests/test_reviewer_reconcile.py +++ b/tests/test_reviewer_reconcile.py @@ -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": ( + '\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 = [ diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 67ee2180..a0d7d17a 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -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: