diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index 127d64cf..253859c0 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -76,10 +76,14 @@ class Finding(TypedDict, total=False): status: FindingStatus first_seen_sha: str last_confirmed_sha: str + github_review_id: int | None github_review_comment_id: int | None + github_review_comment_ids: list[int] github_review_thread_id: str | None + github_review_thread_ids: list[str] github_review_run_id: str | None github_thread_resolved: bool + github_resolved_thread_ids: list[str] last_human_reply_at: str | None last_human_reply_author: str | None last_human_reply_body: str | None @@ -141,10 +145,14 @@ def new_finding( "status": "open", "first_seen_sha": sha, "last_confirmed_sha": sha, + "github_review_id": None, "github_review_comment_id": None, + "github_review_comment_ids": [], "github_review_thread_id": None, + "github_review_thread_ids": [], "github_review_run_id": None, "github_thread_resolved": False, + "github_resolved_thread_ids": [], "last_human_reply_at": None, "last_human_reply_author": None, "last_human_reply_body": None, diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index d98757b8..25db1715 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -22,11 +22,12 @@ from __future__ import annotations import json import logging -from typing import Any +import re +from typing import Any, TypedDict import httpx -from .reviewer_findings import Finding +from .reviewer_findings import DiffSide, Finding from .utils.github_token import GitHubAuthError logger = logging.getLogger(__name__) @@ -35,6 +36,52 @@ logger = logging.getLogger(__name__) _GITHUB_API_BASE = "https://api.github.com" _GITHUB_GRAPHQL = "https://api.github.com/graphql" _GITHUB_HEADERS_VERSION = "2022-11-28" +_OPEN_SWE_REVIEW_COMMENT_MARKER_RE = re.compile( + r"", + re.DOTALL, +) + + +class ReviewCommentMarker(TypedDict): + id: str + file_path: str + start_line: int | None + end_line: int | None + side: DiffSide + + +def _optional_int(value: Any) -> int | None: + return value if isinstance(value, int) else None + + +def parse_review_comment_marker(body: str) -> ReviewCommentMarker | None: + match = _OPEN_SWE_REVIEW_COMMENT_MARKER_RE.search(body) + if match is None: + return None + try: + payload = json.loads(match.group(1)) + except json.JSONDecodeError: + return None + if not isinstance(payload, dict): + return None + + finding_id = payload.get("id") + file_path = payload.get("file_path") + side_raw = payload.get("side", "RIGHT") + if not isinstance(finding_id, str) or not finding_id: + return None + if not isinstance(file_path, str) or not file_path: + return None + if side_raw not in {"LEFT", "RIGHT"}: + return None + side: DiffSide = "LEFT" if side_raw == "LEFT" else "RIGHT" + return { + "id": finding_id, + "file_path": file_path, + "start_line": _optional_int(payload.get("start_line")), + "end_line": _optional_int(payload.get("end_line")), + "side": side, + } def render_inline_comment_body(finding: Finding) -> str: diff --git a/agent/reviewer_reconcile.py b/agent/reviewer_reconcile.py index 51609779..d1fd0fe0 100644 --- a/agent/reviewer_reconcile.py +++ b/agent/reviewer_reconcile.py @@ -3,8 +3,26 @@ from __future__ import annotations from typing import Any from .reviewer_findings import Finding, list_findings, replace_findings +from .reviewer_publish import parse_review_comment_marker ReviewThread = dict[str, Any] +ReviewThreadMatch = tuple[ReviewThread, int | None] + + +def _is_open_swe_bot_comment(comment: ReviewThread) -> bool: + return comment.get("author") == "open-swe[bot]" + + +def _int_list(value: Any) -> list[int]: + if not isinstance(value, list): + return [] + return [item for item in value if isinstance(item, int)] + + +def _str_list(value: Any) -> list[str]: + if not isinstance(value, list): + return [] + return [item for item in value if isinstance(item, str) and item] def _human_replies_after_bot_comment( @@ -36,13 +54,14 @@ def _human_replies_after_bot_comment( def _index_review_threads( review_threads: list[ReviewThread], -) -> tuple[dict[str, ReviewThread], dict[int, ReviewThread]]: +) -> tuple[dict[str, ReviewThread], dict[int, ReviewThread], dict[str, list[ReviewThreadMatch]]]: by_thread_id = { thread_id: review_thread for review_thread in review_threads if isinstance(thread_id := review_thread.get("id"), str) and thread_id } by_comment_id: dict[int, ReviewThread] = {} + by_marker_id: dict[str, list[ReviewThreadMatch]] = {} for review_thread in review_threads: comments = review_thread.get("comments") if not isinstance(comments, list): @@ -53,49 +72,114 @@ def _index_review_threads( comment_id = comment.get("id") if isinstance(comment_id, int): by_comment_id[comment_id] = review_thread - return by_thread_id, by_comment_id + body = comment.get("body") + if not isinstance(body, str) or not _is_open_swe_bot_comment(comment): + continue + marker = parse_review_comment_marker(body) + if marker is not None and isinstance(comment_id, int): + by_marker_id.setdefault(marker["id"], []).append((review_thread, comment_id)) + return by_thread_id, by_comment_id, by_marker_id -def _find_review_thread_for_finding( +def _find_review_threads_for_finding( finding: Finding, *, by_thread_id: dict[str, ReviewThread], by_comment_id: dict[int, ReviewThread], -) -> ReviewThread | None: + by_marker_id: dict[str, list[ReviewThreadMatch]], +) -> list[ReviewThreadMatch]: + finding_id = finding.get("id") + if isinstance(finding_id, str): + marker_match = by_marker_id.get(finding_id) + if marker_match: + return marker_match + github_thread_id = finding.get("github_review_thread_id") if isinstance(github_thread_id, str) and github_thread_id: review_thread = by_thread_id.get(github_thread_id) if review_thread is not None: - return review_thread + comment_id = finding.get("github_review_comment_id") + return [(review_thread, comment_id if isinstance(comment_id, int) else None)] github_comment_id = finding.get("github_review_comment_id") if isinstance(github_comment_id, int): - return by_comment_id.get(github_comment_id) - return None + review_thread = by_comment_id.get(github_comment_id) + if review_thread is not None: + return [(review_thread, github_comment_id)] + return [] -def _sync_thread_status(finding: Finding, review_thread: ReviewThread) -> bool: +def _sync_publication_identity( + finding: Finding, + review_thread: ReviewThread, + comment_id: int | None, +) -> bool: updated = False + if isinstance(comment_id, int) and not isinstance(finding.get("github_review_comment_id"), int): + finding["github_review_comment_id"] = comment_id + updated = True + comment_ids = _int_list(finding.get("github_review_comment_ids")) + if isinstance(comment_id, int) and comment_id not in comment_ids: + comment_ids.append(comment_id) + finding["github_review_comment_ids"] = comment_ids + updated = True + github_thread_id = finding.get("github_review_thread_id") + new_thread_id = review_thread.get("id") if not isinstance(github_thread_id, str) or not github_thread_id: - new_thread_id = review_thread.get("id") if isinstance(new_thread_id, str) and new_thread_id: finding["github_review_thread_id"] = new_thread_id updated = True - - if review_thread.get("is_resolved") or review_thread.get("is_outdated"): - if finding.get("status") == "open": - finding["status"] = "resolved" - finding["last_reconciliation_note"] = "GitHub thread is resolved or outdated." - updated = True - if review_thread.get("is_resolved") and not finding.get("github_thread_resolved"): - finding["github_thread_resolved"] = True - updated = True + thread_ids = _str_list(finding.get("github_review_thread_ids")) + if isinstance(new_thread_id, str) and new_thread_id and new_thread_id not in thread_ids: + thread_ids.append(new_thread_id) + finding["github_review_thread_ids"] = thread_ids + updated = True return updated -def _sync_latest_human_reply(finding: Finding, review_thread: ReviewThread) -> bool: - github_comment_id = finding.get("github_review_comment_id") +def _is_terminal_thread(review_thread: ReviewThread) -> bool: + return bool(review_thread.get("is_resolved") or review_thread.get("is_outdated")) + + +def _sync_thread_status(finding: Finding, matches: list[ReviewThreadMatch]) -> bool: + if not matches or not all(_is_terminal_thread(review_thread) for review_thread, _ in matches): + return False + + updated = False + if finding.get("status") == "open": + finding["status"] = "resolved" + finding["last_reconciliation_note"] = "All GitHub threads are resolved or outdated." + updated = True + + resolved_thread_ids = _str_list(finding.get("github_resolved_thread_ids")) + all_resolved = True + for review_thread, _comment_id in matches: + thread_id = review_thread.get("id") + if review_thread.get("is_resolved") and isinstance(thread_id, str) and thread_id: + if thread_id not in resolved_thread_ids: + resolved_thread_ids.append(thread_id) + updated = True + else: + all_resolved = False + + if resolved_thread_ids != _str_list(finding.get("github_resolved_thread_ids")): + finding["github_resolved_thread_ids"] = resolved_thread_ids + if all_resolved and not finding.get("github_thread_resolved"): + finding["github_thread_resolved"] = True + updated = True + return updated + + +def _sync_latest_human_reply( + finding: Finding, + review_thread: ReviewThread, + *, + comment_id: int | None, +) -> bool: + github_comment_id = ( + comment_id if isinstance(comment_id, int) else finding.get("github_review_comment_id") + ) if not isinstance(github_comment_id, int): return False @@ -130,20 +214,22 @@ async def reconcile_findings_with_review_threads( if not findings: return findings - by_thread_id, by_comment_id = _index_review_threads(review_threads) + by_thread_id, by_comment_id, by_marker_id = _index_review_threads(review_threads) updated = False for finding in findings: - review_thread = _find_review_thread_for_finding( + matches = _find_review_threads_for_finding( finding, by_thread_id=by_thread_id, by_comment_id=by_comment_id, + by_marker_id=by_marker_id, ) - if review_thread is None: - continue - - updated = _sync_thread_status(finding, review_thread) or updated - updated = _sync_latest_human_reply(finding, review_thread) or updated + for review_thread, comment_id in matches: + updated = _sync_publication_identity(finding, review_thread, comment_id) or updated + updated = ( + _sync_latest_human_reply(finding, review_thread, comment_id=comment_id) or updated + ) + updated = _sync_thread_status(finding, matches) or updated if updated: await replace_findings(reviewer_thread_id, findings) diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 8b158376..e3efcec9 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -9,6 +9,7 @@ from langgraph.config import get_config from ..reviewer_diff import compute_diff_line_set, fetch_pr_diff, is_range_in_diff from ..reviewer_findings import ( + Finding, Severity, filter_findings_for_publish, get_thread_id_from_runtime, @@ -24,11 +25,13 @@ from ..reviewer_publish import ( fetch_pr_review_threads, fetch_review_comments, fetch_review_thread_id_for_comment, + parse_review_comment_marker, post_pull_request_review, render_inline_comment_payload, render_review_body, resolve_review_thread, ) +from ..reviewer_reconcile import reconcile_findings_with_review_threads from ..utils.github_token import ( GitHubAuthError, get_github_token, @@ -142,9 +145,7 @@ async def _publish_review_eval_dry_run_async( """Simulate publish_review for benchmark runs without posting to GitHub.""" thread_id = get_thread_id_from_runtime() findings = await list_findings_async(thread_id) - unpublished_findings = [ - f for f in findings if not isinstance(f.get("github_review_comment_id"), int) - ] + unpublished_findings = [f for f in findings if not _has_publication_identity(f)] open_unpublished = [f for f in unpublished_findings if f.get("status", "open") == "open"] eligible = filter_findings_for_publish( unpublished_findings, @@ -182,7 +183,13 @@ async def _publish_review_async( langgraph_run_id: str | None = None, ) -> dict[str, Any]: thread_id = get_thread_id_from_runtime() - findings = await list_findings_async(thread_id) + findings = await _backfill_findings_from_pr_threads( + thread_id=thread_id, + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + ) # Re-reviews only post NEW findings. Anything with a github_review_comment_id # already lives on GitHub from a prior publish — reposting would create @@ -191,6 +198,10 @@ async def _publish_review_async( unpublished_findings = [ f for f in findings if not isinstance(f.get("github_review_comment_id"), int) ] + if is_re_review: + unpublished_findings = [ + f for f in unpublished_findings if f.get("first_seen_sha") == head_sha + ] open_unpublished = [f for f in unpublished_findings if f.get("status", "open") == "open"] eligible = filter_findings_for_publish( unpublished_findings, severity_threshold=severity_threshold, cap=cap @@ -319,6 +330,12 @@ async def _publish_review_async( review_id = review_response.get("id") if isinstance(review_response, dict) else None if review_id is not None and inline_comments: + await _store_review_id_on_findings( + thread_id=thread_id, + findings=findings, + eligible_with_payload=eligible_with_payload, + review_id=review_id, + ) comment_records = await fetch_review_comments( owner=owner, repo=repo, @@ -338,6 +355,15 @@ async def _publish_review_async( comment_records=comment_records, langgraph_run_id=langgraph_run_id, ) + current_findings = await list_findings_async(thread_id) + if _missing_comment_ids_for_published_findings(current_findings, eligible_with_payload): + await _backfill_findings_from_pr_threads( + thread_id=thread_id, + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + ) await _store_thread_ids_on_findings( thread_id=thread_id, owner=owner, @@ -382,6 +408,99 @@ async def _publish_review_async( return result +def _has_publication_identity(finding: Finding) -> bool: + return isinstance(finding.get("github_review_comment_id"), int) or isinstance( + finding.get("github_review_id"), int + ) + + +def _int_list(value: Any) -> list[int]: + if not isinstance(value, list): + return [] + return [item for item in value if isinstance(item, int)] + + +def _str_list(value: Any) -> list[str]: + if not isinstance(value, list): + return [] + return [item for item in value if isinstance(item, str) and item] + + +def _comment_ids_for_finding(finding: dict[str, Any]) -> list[int]: + comment_ids = _int_list(finding.get("github_review_comment_ids")) + comment_id = finding.get("github_review_comment_id") + if isinstance(comment_id, int) and comment_id not in comment_ids: + comment_ids.insert(0, comment_id) + return comment_ids + + +def _thread_ids_for_finding(finding: dict[str, Any]) -> list[str]: + thread_ids = _str_list(finding.get("github_review_thread_ids")) + thread_id = finding.get("github_review_thread_id") + if isinstance(thread_id, str) and thread_id and thread_id not in thread_ids: + thread_ids.insert(0, thread_id) + return thread_ids + + +async def _backfill_findings_from_pr_threads( + *, + thread_id: str, + owner: str, + repo: str, + pr_number: int, + token: str, +) -> list[Finding]: + findings = await list_findings_async(thread_id) + if not findings: + return findings + review_threads = await fetch_pr_review_threads( + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + ) + if not review_threads: + return findings + return await reconcile_findings_with_review_threads(thread_id, review_threads) + + +def _missing_comment_ids_for_published_findings( + findings: list[Finding], + eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]], +) -> bool: + finding_ids = { + finding.get("id") + for finding, _payload in eligible_with_payload + if isinstance(finding.get("id"), str) + } + for finding in findings: + if finding.get("id") in finding_ids and not isinstance( + finding.get("github_review_comment_id"), int + ): + return True + return False + + +async def _store_review_id_on_findings( + *, + thread_id: str, + findings: list[Finding], + eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]], + review_id: int, +) -> None: + finding_ids = { + finding.get("id") + for finding, _payload in eligible_with_payload + if isinstance(finding.get("id"), str) + } + updated = False + for finding in findings: + if finding.get("id") in finding_ids and finding.get("github_review_id") != review_id: + finding["github_review_id"] = review_id + if updated: + await replace_findings(thread_id, findings) + + async def _resolve_diff_line_set( *, owner: str, @@ -493,16 +612,17 @@ async def _maybe_post_slack_completion_reply( async def _store_comment_ids_on_findings( *, thread_id: str, - findings: list[dict[str, Any]], + findings: list[Finding], eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]], comment_records: list[dict[str, Any]], langgraph_run_id: str | None, ) -> None: """Match returned GitHub comment ids back to the findings that produced them. - Match key is ``(path, line, body)`` since we don't have a server-side hint - pointing each REST comment to its source finding. + Prefer the Open SWE marker because it survives body formatting changes; + fall back to ``(path, line, body)`` for older comments. """ + by_marker_id: dict[str, int] = {} by_key: dict[tuple[str, int, str], int] = {} for record in comment_records: path = record.get("path") @@ -516,25 +636,33 @@ async def _store_comment_ids_on_findings( and isinstance(comment_id, int) ): by_key[(path, line, body)] = comment_id + marker = parse_review_comment_marker(body) + if marker is not None: + by_marker_id[marker["id"]] = comment_id updated = False findings_by_id = {f.get("id"): f for f in findings} for finding_snapshot, payload in eligible_with_payload: + finding_id = finding_snapshot.get("id") + comment_id = by_marker_id.get(finding_id) if isinstance(finding_id, str) else None line_value = payload.get("line") - if not isinstance(line_value, int): - continue - key = ( - str(payload.get("path", "")), - line_value, - str(payload.get("body", "")), - ) - comment_id = by_key.get(key) + if comment_id is None and isinstance(line_value, int): + key = ( + str(payload.get("path", "")), + line_value, + str(payload.get("body", "")), + ) + comment_id = by_key.get(key) if comment_id is None: continue - finding = findings_by_id.get(finding_snapshot.get("id")) + finding = findings_by_id.get(finding_id) if finding is None: continue finding["github_review_comment_id"] = comment_id + comment_ids = _int_list(finding.get("github_review_comment_ids")) + if comment_id not in comment_ids: + comment_ids.append(comment_id) + finding["github_review_comment_ids"] = comment_ids if langgraph_run_id: finding["github_review_run_id"] = langgraph_run_id updated = True @@ -552,16 +680,12 @@ async def _store_thread_ids_on_findings( token: str, ) -> None: findings = await list_findings_async(thread_id) - comment_ids_by_finding_id: dict[str, int] = {} + comment_ids_by_finding_id: dict[str, list[int]] = {} for finding in findings: finding_id = finding.get("id") - comment_id = finding.get("github_review_comment_id") - if ( - isinstance(finding_id, str) - and isinstance(comment_id, int) - and not isinstance(finding.get("github_review_thread_id"), str) - ): - comment_ids_by_finding_id[finding_id] = comment_id + comment_ids = _comment_ids_for_finding(finding) + if isinstance(finding_id, str) and comment_ids and not _thread_ids_for_finding(finding): + comment_ids_by_finding_id[finding_id] = comment_ids if not comment_ids_by_finding_id: return @@ -588,12 +712,18 @@ async def _store_thread_ids_on_findings( finding_id = finding.get("id") if not isinstance(finding_id, str): continue - comment_id = comment_ids_by_finding_id.get(finding_id) - if not isinstance(comment_id, int): - continue - github_thread_id = thread_id_by_comment_id.get(comment_id) - if github_thread_id: - finding["github_review_thread_id"] = github_thread_id + thread_ids = _thread_ids_for_finding(finding) + for comment_id in comment_ids_by_finding_id.get(finding_id, []): + github_thread_id = thread_id_by_comment_id.get(comment_id) + if not github_thread_id: + continue + if not isinstance(finding.get("github_review_thread_id"), str): + finding["github_review_thread_id"] = github_thread_id + updated = True + if github_thread_id not in thread_ids: + thread_ids.append(github_thread_id) + finding["github_review_thread_ids"] = thread_ids + updated = True updated = True if updated: @@ -610,24 +740,17 @@ async def _resolve_threads_for_resolved_findings( ) -> int: """Resolve GitHub review threads for findings that just transitioned to resolved. - A finding qualifies if: - - status == ``resolved`` - - has a ``github_review_comment_id`` from a prior publish - - has not already been GitHub-resolved (tracked via - ``github_thread_resolved`` flag we write back here) + Resolves every known GitHub thread for a finding. Multiple threads can + exist when an earlier run duplicated a comment before publication identity + was backfilled. """ resolved_count = 0 mutated = False for finding in findings: if finding.get("status") != "resolved": continue - comment_id = finding.get("github_review_comment_id") - if not isinstance(comment_id, int): - continue - if finding.get("github_thread_resolved"): - continue - thread_node_id = finding.get("github_review_thread_id") - if not isinstance(thread_node_id, str) or not thread_node_id: + thread_node_ids = _thread_ids_for_finding(finding) + for comment_id in _comment_ids_for_finding(finding): thread_node_id = await fetch_review_thread_id_for_comment( owner=owner, repo=repo, @@ -635,13 +758,32 @@ async def _resolve_threads_for_resolved_findings( review_comment_id=comment_id, token=token, ) - if not thread_node_id: + if thread_node_id and thread_node_id not in thread_node_ids: + thread_node_ids.append(thread_node_id) + + if not thread_node_ids: continue - ok = await resolve_review_thread(thread_node_id=thread_node_id, token=token) - if ok: + + resolved_thread_ids = _str_list(finding.get("github_resolved_thread_ids")) + for thread_node_id in thread_node_ids: + if thread_node_id in resolved_thread_ids: + continue + ok = await resolve_review_thread(thread_node_id=thread_node_id, token=token) + if ok: + resolved_thread_ids.append(thread_node_id) + resolved_count += 1 + mutated = True + + if resolved_thread_ids: + finding["github_resolved_thread_ids"] = resolved_thread_ids + if thread_node_ids: + finding["github_review_thread_ids"] = thread_node_ids + if not isinstance(finding.get("github_review_thread_id"), str): + finding["github_review_thread_id"] = thread_node_ids[0] + if thread_node_ids and all( + thread_id in resolved_thread_ids for thread_id in thread_node_ids + ): finding["github_thread_resolved"] = True - mutated = True - resolved_count += 1 if mutated: thread_id = get_thread_id_from_runtime() diff --git a/agent/tools/resolve_finding_thread.py b/agent/tools/resolve_finding_thread.py index 8fbb5e2b..044d4c7b 100644 --- a/agent/tools/resolve_finding_thread.py +++ b/agent/tools/resolve_finding_thread.py @@ -5,8 +5,18 @@ from typing import Any from langgraph.config import get_config -from ..reviewer_findings import get_finding, get_thread_id_from_runtime, update_finding_fields -from ..reviewer_publish import fetch_review_thread_id_for_comment, resolve_review_thread +from ..reviewer_findings import ( + Finding, + get_finding, + get_thread_id_from_runtime, + update_finding_fields, +) +from ..reviewer_publish import ( + fetch_pr_review_threads, + fetch_review_thread_id_for_comment, + resolve_review_thread, +) +from ..reviewer_reconcile import reconcile_findings_with_review_threads from ..utils.github_token import get_github_token @@ -64,35 +74,110 @@ async def _resolve_finding_thread_async( token: str, ) -> dict[str, Any]: thread_id = get_thread_id_from_runtime() - finding = await get_finding(thread_id, finding_id) + finding = await _get_finding_with_pr_backfill( + thread_id=thread_id, + finding_id=finding_id, + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + ) if finding is None: return {"success": False, "error": f"No finding found with id {finding_id}"} - github_thread_id = finding.get("github_review_thread_id") - if not isinstance(github_thread_id, str) or not github_thread_id: - comment_id = finding.get("github_review_comment_id") - if not isinstance(comment_id, int): - return {"success": False, "error": "Finding has no GitHub review thread mapping"} - github_thread_id = await fetch_review_thread_id_for_comment( + github_thread_ids = _thread_ids_for_finding(finding) + for comment_id in _comment_ids_for_finding(finding): + thread_node_id = await fetch_review_thread_id_for_comment( owner=owner, repo=repo, pr_number=pr_number, review_comment_id=comment_id, token=token, ) - if not github_thread_id: + if thread_node_id and thread_node_id not in github_thread_ids: + github_thread_ids.append(thread_node_id) + if not github_thread_ids: return {"success": False, "error": "Could not resolve GitHub review thread id"} - ok = await resolve_review_thread(thread_node_id=github_thread_id, token=token) - if not ok: + resolved_thread_ids = _str_list(finding.get("github_resolved_thread_ids")) + resolved_count = 0 + for github_thread_id in github_thread_ids: + if github_thread_id in resolved_thread_ids: + continue + ok = await resolve_review_thread(thread_node_id=github_thread_id, token=token) + if ok: + resolved_thread_ids.append(github_thread_id) + resolved_count += 1 + if resolved_count == 0 and not all( + github_thread_id in resolved_thread_ids for github_thread_id in github_thread_ids + ): return {"success": False, "error": "GitHub did not resolve the review thread"} updates: dict[str, Any] = { "status": status, - "github_review_thread_id": github_thread_id, - "github_thread_resolved": True, + "github_review_thread_id": github_thread_ids[0], + "github_review_thread_ids": github_thread_ids, + "github_resolved_thread_ids": resolved_thread_ids, + "github_thread_resolved": all( + github_thread_id in resolved_thread_ids for github_thread_id in github_thread_ids + ), } if note: updates["last_reconciliation_note"] = note updated = await update_finding_fields(thread_id, finding_id, updates) - return {"success": True, "finding": updated} + return {"success": True, "finding": updated, "resolved_thread_count": resolved_count} + + +async def _get_finding_with_pr_backfill( + *, + thread_id: str, + finding_id: str, + owner: str, + repo: str, + pr_number: int, + token: str, +) -> Finding | None: + finding = await get_finding(thread_id, finding_id) + if finding is None: + return None + if _thread_ids_for_finding(finding) or _comment_ids_for_finding(finding): + return finding + + review_threads = await fetch_pr_review_threads( + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + ) + if review_threads: + await reconcile_findings_with_review_threads(thread_id, review_threads) + finding = await get_finding(thread_id, finding_id) + return finding + + +def _int_list(value: Any) -> list[int]: + if not isinstance(value, list): + return [] + return [item for item in value if isinstance(item, int)] + + +def _str_list(value: Any) -> list[str]: + if not isinstance(value, list): + return [] + return [item for item in value if isinstance(item, str) and item] + + +def _comment_ids_for_finding(finding: dict[str, Any]) -> list[int]: + comment_ids = _int_list(finding.get("github_review_comment_ids")) + comment_id = finding.get("github_review_comment_id") + if isinstance(comment_id, int) and comment_id not in comment_ids: + comment_ids.insert(0, comment_id) + return comment_ids + + +def _thread_ids_for_finding(finding: dict[str, Any]) -> list[str]: + thread_ids = _str_list(finding.get("github_review_thread_ids")) + thread_id = finding.get("github_review_thread_id") + if isinstance(thread_id, str) and thread_id and thread_id not in thread_ids: + thread_ids.insert(0, thread_id) + return thread_ids diff --git a/tests/test_reviewer_findings.py b/tests/test_reviewer_findings.py index 8522cf5b..5e3c00df 100644 --- a/tests/test_reviewer_findings.py +++ b/tests/test_reviewer_findings.py @@ -48,10 +48,14 @@ def test_new_finding_defaults() -> None: 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["suggestion"] is None diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 03360eca..dd14c23f 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Iterator from typing import Any from unittest.mock import AsyncMock, MagicMock, patch @@ -10,6 +11,7 @@ import pytest from agent.reviewer_findings import Finding, new_finding from agent.reviewer_publish import ( fetch_pr_review_threads, + parse_review_comment_marker, post_pull_request_review, render_inline_comment_body, render_inline_comment_payload, @@ -34,6 +36,15 @@ def _f(**overrides: Any) -> Finding: return base +@pytest.fixture(autouse=True) +def _isolate_publish_review_pr_state() -> Iterator[None]: + with ( + patch("agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=[])), + patch("agent.tools.publish_review.replace_findings", AsyncMock()), + ): + yield + + def test_render_inline_comment_body_without_suggestion() -> None: body = render_inline_comment_body(_f(description="just text")) assert "") is None + assert ( + parse_review_comment_marker( + '' + ) + is None + ) + + def test_render_inline_comment_payload_single_line() -> None: payload = render_inline_comment_payload(_f(start_line=10, end_line=10)) assert payload is not None @@ -313,6 +354,247 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> assert result["skipped_empty_re_review"] is True +@pytest.mark.asyncio +async def test_re_review_backfills_existing_marker_and_skips_duplicate_post() -> None: + from agent.tools.publish_review import _publish_review_async + + finding = _f(id="f_old", first_seen_sha="oldsha", github_review_comment_id=None) + findings = [finding] + thread = { + "id": "THREAD_1", + "is_resolved": False, + "is_outdated": False, + "comments": [ + { + "id": 101, + "author": "open-swe[bot]", + "body": render_inline_comment_body(finding), + "created_at": "2026-05-27T10:00:00Z", + } + ], + } + post_review = AsyncMock() + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch( + "agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=[thread]) + ), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)), + patch("agent.reviewer_reconcile.list_findings", AsyncMock(return_value=findings)), + patch("agent.reviewer_reconcile.replace_findings", AsyncMock()), + patch("agent.tools.publish_review.post_pull_request_review", post_review), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="newsha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=True, + ) + + post_review.assert_not_called() + assert result["skipped_empty_re_review"] is True + assert findings[0]["github_review_comment_id"] == 101 + assert findings[0]["github_review_thread_id"] == "THREAD_1" + + +@pytest.mark.asyncio +async def test_re_review_backfills_and_resolves_duplicate_existing_threads() -> None: + from agent.tools.publish_review import _publish_review_async + + finding = _f( + id="f_old", + first_seen_sha="oldsha", + github_review_comment_id=None, + status="resolved", + ) + findings = [finding] + threads = [ + { + "id": "THREAD_1", + "is_resolved": False, + "is_outdated": False, + "comments": [ + { + "id": 101, + "author": "open-swe[bot]", + "body": render_inline_comment_body(finding), + "created_at": "2026-05-27T10:00:00Z", + } + ], + }, + { + "id": "THREAD_2", + "is_resolved": False, + "is_outdated": False, + "comments": [ + { + "id": 102, + "author": "open-swe[bot]", + "body": render_inline_comment_body(finding), + "created_at": "2026-05-27T10:01:00Z", + } + ], + }, + ] + resolve_thread = AsyncMock(return_value=True) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch( + "agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=threads) + ), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)), + patch("agent.reviewer_reconcile.list_findings", AsyncMock(return_value=findings)), + patch("agent.reviewer_reconcile.replace_findings", AsyncMock()), + patch("agent.tools.publish_review.post_pull_request_review", AsyncMock()), + patch("agent.tools.publish_review.resolve_review_thread", resolve_thread), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="newsha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=True, + ) + + assert result["success"] is True + assert result["review_id"] is None + assert result["resolved_thread_count"] == 2 + assert resolve_thread.await_count == 2 + assert findings[0]["github_review_comment_ids"] == [101, 102] + assert findings[0]["github_review_thread_ids"] == ["THREAD_1", "THREAD_2"] + assert findings[0]["github_resolved_thread_ids"] == ["THREAD_1", "THREAD_2"] + assert findings[0]["github_thread_resolved"] is True + + +@pytest.mark.asyncio +async def test_publish_review_backfills_from_threads_when_review_comments_are_empty() -> None: + from agent.tools.publish_review import _publish_review_async + + finding = _f(id="f_new", first_seen_sha="sha") + findings = [finding] + thread = { + "id": "THREAD_1", + "is_resolved": False, + "is_outdated": False, + "comments": [ + { + "id": 202, + "author": "open-swe[bot]", + "body": render_inline_comment_body(finding), + "created_at": "2026-05-27T10:00:00Z", + } + ], + } + fetch_threads = AsyncMock(side_effect=[[], [thread]]) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.fetch_pr_review_threads", fetch_threads), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)), + patch("agent.reviewer_reconcile.list_findings", AsyncMock(return_value=findings)), + patch("agent.reviewer_reconcile.replace_findings", AsyncMock()), + patch( + "agent.tools.publish_review.post_pull_request_review", + AsyncMock(return_value={"id": 999}), + ), + patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + patch( + "agent.tools.publish_review._maybe_post_slack_completion_reply", + new_callable=AsyncMock, + ), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + ) + + assert result["success"] is True + assert result["review_id"] == 999 + assert fetch_threads.await_count == 2 + assert findings[0]["github_review_id"] == 999 + assert findings[0]["github_review_comment_id"] == 202 + assert findings[0]["github_review_thread_id"] == "THREAD_1" + + +@pytest.mark.asyncio +async def test_re_review_only_posts_current_head_unpublished_findings() -> None: + from agent.tools.publish_review import _publish_review_async + + old = _f(id="f_old", first_seen_sha="oldsha", file="old.py") + new = _f(id="f_new", first_seen_sha="newsha", file="new.py") + findings = [old, new] + post_review = AsyncMock(return_value={"id": 888}) + fetch_comments = AsyncMock( + return_value=[ + { + "id": 303, + "path": "new.py", + "line": 10, + "body": render_inline_comment_body(new), + } + ] + ) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)), + patch("agent.tools.publish_review.post_pull_request_review", post_review), + patch("agent.tools.publish_review.fetch_review_comments", fetch_comments), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="newsha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=True, + ) + + assert result["success"] is True + assert result["surfaced_count"] == 1 + inline_comments = post_review.await_args.kwargs["inline_comments"] + assert [comment["path"] for comment in inline_comments] == ["new.py"] + assert old["github_review_id"] is None + assert new["github_review_id"] == 888 + assert new["github_review_comment_id"] == 303 + + @pytest.mark.asyncio async def test_publish_review_posts_summary_when_no_findings() -> None: """An empty findings list must still post a review so the user sees feedback.""" diff --git a/tests/test_reviewer_reconcile.py b/tests/test_reviewer_reconcile.py index 148e8b50..2bfc155d 100644 --- a/tests/test_reviewer_reconcile.py +++ b/tests/test_reviewer_reconcile.py @@ -40,6 +40,185 @@ async def test_reconcile_marks_resolved_github_thread_resolved() -> None: replace.assert_awaited_once() +@pytest.mark.asyncio +async def test_reconcile_backfills_comment_and_thread_ids_from_bot_marker() -> 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[bot]", + "body": ( + '\n\nbug' + ), + } + ], + } + ], + ) + + assert result[0]["github_review_comment_id"] == 11 + assert result[0]["github_review_comment_ids"] == [11] + assert result[0]["github_review_thread_id"] == "THREAD_1" + assert result[0]["github_review_thread_ids"] == ["THREAD_1"] + replace.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_reconcile_duplicate_markers_require_all_threads_terminal() -> None: + findings = [ + { + "id": "f1", + "status": "open", + "github_review_comment_id": None, + "github_review_thread_id": None, + } + ] + replace = AsyncMock() + marker = ( + '\n\nbug' + ) + + 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_OLD", + "is_resolved": False, + "is_outdated": True, + "comments": [{"id": 11, "author": "open-swe[bot]", "body": marker}], + }, + { + "id": "THREAD_OPEN", + "is_resolved": False, + "is_outdated": False, + "comments": [{"id": 12, "author": "open-swe[bot]", "body": marker}], + }, + ], + ) + + assert result[0]["status"] == "open" + assert result[0]["github_review_comment_ids"] == [11, 12] + assert result[0]["github_review_thread_ids"] == ["THREAD_OLD", "THREAD_OPEN"] + replace.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_reconcile_duplicate_markers_resolve_when_all_threads_terminal() -> None: + findings = [ + { + "id": "f1", + "status": "open", + "github_review_comment_id": None, + "github_review_thread_id": None, + } + ] + replace = AsyncMock() + marker = ( + '\n\nbug' + ) + + 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_OLD", + "is_resolved": False, + "is_outdated": True, + "comments": [{"id": 11, "author": "open-swe[bot]", "body": marker}], + }, + { + "id": "THREAD_RESOLVED", + "is_resolved": True, + "is_outdated": False, + "comments": [{"id": 12, "author": "open-swe[bot]", "body": marker}], + }, + ], + ) + + assert result[0]["status"] == "resolved" + assert result[0]["last_reconciliation_note"] == "All GitHub threads are resolved or outdated." + assert result[0]["github_resolved_thread_ids"] == ["THREAD_RESOLVED"] + assert result[0].get("github_thread_resolved") is not True + replace.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_reconcile_ignores_spoofed_non_bot_marker() -> 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": "human", + "body": ( + '\n\nspoof' + ), + } + ], + } + ], + ) + + assert result[0]["github_review_comment_id"] is None + assert result[0]["github_review_thread_id"] is None + replace.assert_not_awaited() + + @pytest.mark.asyncio async def test_reconcile_records_latest_human_reply_after_bot_comment() -> None: findings = [{"id": "f1", "status": "open", "github_review_comment_id": 11}] diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 7df2dd28..2fc71611 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -7,6 +7,7 @@ from unittest.mock import AsyncMock, patch from agent.tools.add_finding import add_finding from agent.tools.list_findings import list_findings +from agent.tools.resolve_finding_thread import resolve_finding_thread from agent.tools.update_finding import update_finding @@ -196,6 +197,40 @@ def test_update_finding_rejects_invalid_status() -> None: assert result["success"] is False +def test_resolve_finding_thread_resolves_all_known_threads() -> None: + finding = { + "id": "f1", + "status": "open", + "github_review_thread_ids": ["THREAD_1", "THREAD_2"], + "github_review_comment_ids": [11, 12], + } + update = AsyncMock(return_value={**finding, "status": "resolved"}) + resolve = AsyncMock(return_value=True) + + with ( + patch( + "agent.tools.resolve_finding_thread.get_config", + return_value=_config(repo={"owner": "o", "name": "r"}, pr_number=7), + ), + patch("agent.tools.resolve_finding_thread.get_github_token", return_value="token"), + patch("agent.tools.resolve_finding_thread.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.resolve_finding_thread.get_finding", AsyncMock(return_value=finding)), + patch("agent.tools.resolve_finding_thread.resolve_review_thread", resolve), + patch("agent.tools.resolve_finding_thread.update_finding_fields", update), + ): + result = resolve_finding_thread("f1", status="resolved") + + assert result["success"] is True + assert result["resolved_thread_count"] == 2 + assert [call.kwargs["thread_node_id"] for call in resolve.await_args_list] == [ + "THREAD_1", + "THREAD_2", + ] + updates = update.await_args.args[2] + assert updates["github_thread_resolved"] is True + assert updates["github_resolved_thread_ids"] == ["THREAD_1", "THREAD_2"] + + def test_update_finding_rejects_empty_update() -> None: with patch("agent.tools.update_finding.get_config", return_value=_config()): result = update_finding(finding_id="f_x")