mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 09:13:14 +00:00
fix: dedupe reviewer comments from PR state (#1341)
* fix: dedupe reviewer comments from PR state Use GitHub review-thread markers to repair reviewer publication state before posting or resolving findings, so re-reviews do not duplicate comments and resolved findings close all matching PR threads. * fix: require all duplicate reviewer threads resolved Avoid treating a marker-backed finding as resolved when only one duplicate thread is outdated while another matching thread remains open.
This commit is contained in:
Johannes du Plessis
GitHub
parent
dd4ca9181e
commit
0d4d1c5a3b
9 changed files with 959 additions and 91 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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"<!--\s*open-swe-review-comment\s+(\{.*?\})\s*-->",
|
||||
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:
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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 "<!-- open-swe-review-comment" in body
|
||||
|
|
@ -51,6 +62,36 @@ def test_render_inline_comment_body_with_suggestion_appends_block() -> None:
|
|||
assert "x = 1\nx += 1" in body
|
||||
|
||||
|
||||
def test_parse_review_comment_marker_accepts_valid_marker() -> None:
|
||||
finding = _f(
|
||||
id="f_marker",
|
||||
file="agent/webapp.py",
|
||||
start_line=10,
|
||||
end_line=12,
|
||||
side="RIGHT",
|
||||
)
|
||||
marker = parse_review_comment_marker(render_inline_comment_body(finding))
|
||||
|
||||
assert marker == {
|
||||
"id": "f_marker",
|
||||
"file_path": "agent/webapp.py",
|
||||
"start_line": 10,
|
||||
"end_line": 12,
|
||||
"side": "RIGHT",
|
||||
}
|
||||
|
||||
|
||||
def test_parse_review_comment_marker_rejects_malformed_marker() -> None:
|
||||
assert parse_review_comment_marker("plain body") is None
|
||||
assert parse_review_comment_marker("<!-- open-swe-review-comment {} -->") is None
|
||||
assert (
|
||||
parse_review_comment_marker(
|
||||
'<!-- open-swe-review-comment {"id":"f1","file_path":"x.py","side":"BAD"} -->'
|
||||
)
|
||||
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."""
|
||||
|
|
|
|||
|
|
@ -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": (
|
||||
'<!-- 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_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 = (
|
||||
'<!-- open-swe-review-comment {"id":"f1",'
|
||||
'"file_path":"a.py","start_line":1,'
|
||||
'"end_line":1,"side":"RIGHT"} -->\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 = (
|
||||
'<!-- open-swe-review-comment {"id":"f1",'
|
||||
'"file_path":"a.py","start_line":1,'
|
||||
'"end_line":1,"side":"RIGHT"} -->\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": (
|
||||
'<!-- open-swe-review-comment {"id":"f1",'
|
||||
'"file_path":"a.py","start_line":1,'
|
||||
'"end_line":1,"side":"RIGHT"} -->\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}]
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue