mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 10:23:14 +00:00
feat: structured reviewer comments + auto resolution comments (#1352)
* feat: structured reviewer comments + auto resolution comments Restructure inline review comment bodies (severity emoji, bold title from the first line, line reference, feedback footer) without duplicating the first description line, and post an automatic resolution/dismissal comment to the GitHub thread when a finding is resolved or dismissed. - Centralize render_resolution_comment in reviewer_publish; fix a crash when last_reconciliation_note is None and drop the misleading generic fallback. - Post the resolution comment in resolve_finding_thread (the normal update_finding path), not only in publish_review, so it actually fires on re-review. Dedupe via github_posted_resolution_comment_ids. - Add pytest coverage for rendering and the resolution-comment flow. * fix: post resolution comment to every closed thread in resolve_finding_thread Per-thread iteration (matching _resolve_threads_for_resolved_findings) so duplicate threads after the first also receive the resolved/dismissed explanation before being closed.
This commit is contained in:
Johannes du Plessis
GitHub
parent
4e6c445246
commit
fee209601b
7 changed files with 218 additions and 20 deletions
|
|
@ -101,17 +101,23 @@ If `publish_review` returns `unresolvable_findings`, do NOT retry with the
|
|||
same args — call `update_finding(status="resolved")` on those ids, or fix
|
||||
their file/line via `update_finding`, then call `publish_review` again.
|
||||
|
||||
Re-review: for each open finding, `update_finding(id, status="resolved")` if
|
||||
fixed, `update_finding` with new fields + `note` if changed, otherwise do
|
||||
nothing. Add net-new findings with `add_finding`.
|
||||
Re-review: for each open finding, `update_finding(id, status="resolved", note="...")`
|
||||
if fixed (include a brief explanation of the fix in `note`), `update_finding` with
|
||||
new fields + `note` if changed, otherwise do nothing. Add net-new findings with
|
||||
`add_finding`.
|
||||
|
||||
When you mark a finding as resolved, `publish_review` will automatically post a
|
||||
resolution comment to the GitHub thread explaining what was fixed, then close it.
|
||||
The `note` field you provide in `update_finding` becomes part of that comment, so
|
||||
be specific: "The current code at line X now does Y" beats "This is fixed".
|
||||
|
||||
If a human reply shows one of your published findings is invalid, call
|
||||
`resolve_finding_thread(finding_id, status="dismissed")` after verifying the
|
||||
claim. If the finding is fixed by code, use `update_finding(...,
|
||||
status="resolved")`; `publish_review` will close the GitHub thread. Reply with
|
||||
`reply_to_finding_thread` only when the user directly asks a question or a short
|
||||
clarification is needed after pushback. Bias strongly toward resolving/dismissing
|
||||
without replying.
|
||||
`resolve_finding_thread(finding_id, status="dismissed", note="...")` after verifying
|
||||
the claim (the note should explain why). If the finding is fixed by code, use
|
||||
`update_finding(..., status="resolved", note="...")`. Do NOT use
|
||||
`reply_to_finding_thread` for resolutions or dismissals — the system posts those
|
||||
automatically. Use `reply_to_finding_thread` only when the user directly asks a
|
||||
question or a short clarification is needed after pushback.
|
||||
|
||||
# The bar: file a finding only if it passes these criteria
|
||||
|
||||
|
|
|
|||
|
|
@ -86,6 +86,7 @@ class Finding(TypedDict, total=False):
|
|||
github_review_run_id: str | None
|
||||
github_thread_resolved: bool
|
||||
github_resolved_thread_ids: list[str]
|
||||
github_posted_resolution_comment_ids: list[int]
|
||||
last_human_reply_at: str | None
|
||||
last_human_reply_author: str | None
|
||||
last_human_reply_body: str | None
|
||||
|
|
@ -206,6 +207,7 @@ def new_finding(
|
|||
"github_review_run_id": None,
|
||||
"github_thread_resolved": False,
|
||||
"github_resolved_thread_ids": [],
|
||||
"github_posted_resolution_comment_ids": [],
|
||||
"last_human_reply_at": None,
|
||||
"last_human_reply_author": None,
|
||||
"last_human_reply_body": None,
|
||||
|
|
|
|||
|
|
@ -89,7 +89,16 @@ def render_inline_comment_body(finding: Finding) -> str:
|
|||
|
||||
Format:
|
||||
|
||||
<description>
|
||||
<!-- metadata marker -->
|
||||
|
||||
🟡 **Title (first line of the description)**
|
||||
|
||||
<remaining description detail>
|
||||
|
||||
*(Refers to lines X-Y)*
|
||||
|
||||
---
|
||||
*Was this helpful? React with 👍 or 👎 to provide feedback.*
|
||||
|
||||
```suggestion
|
||||
<replacement>
|
||||
|
|
@ -98,7 +107,8 @@ def render_inline_comment_body(finding: Finding) -> str:
|
|||
The suggestion block is only included when ``finding.suggestion`` is set.
|
||||
Multi-line suggestions just become multi-line ```suggestion``` blocks.
|
||||
"""
|
||||
description = finding.get("description", "") or ""
|
||||
description = (finding.get("description") or "").strip()
|
||||
severity = finding.get("severity") or "medium"
|
||||
marker_payload = {
|
||||
"id": finding.get("id", ""),
|
||||
"file_path": finding.get("file", ""),
|
||||
|
|
@ -107,13 +117,78 @@ def render_inline_comment_body(finding: Finding) -> str:
|
|||
"side": finding.get("side", "RIGHT"),
|
||||
}
|
||||
marker = f"<!-- open-swe-review-comment {json.dumps(marker_payload, separators=(',', ':'))} -->"
|
||||
|
||||
title, detail = _split_title_and_detail(description)
|
||||
line_ref = _format_line_reference(finding.get("start_line"), finding.get("end_line"))
|
||||
|
||||
body_parts = [marker, "", f"{_severity_emoji(severity)} **{title}**"]
|
||||
if detail:
|
||||
body_parts.extend(["", detail])
|
||||
if line_ref:
|
||||
body_parts.extend(["", line_ref])
|
||||
body_parts.extend(["", "---", "*Was this helpful? React with 👍 or 👎 to provide feedback.*"])
|
||||
body = "\n".join(body_parts)
|
||||
|
||||
suggestion = finding.get("suggestion")
|
||||
body = f"{marker}\n\n{description}"
|
||||
if suggestion:
|
||||
body = f"{body}\n\n```suggestion\n{suggestion}\n```"
|
||||
return body
|
||||
|
||||
|
||||
def _severity_emoji(severity: str) -> str:
|
||||
return {
|
||||
"critical": "🔴",
|
||||
"high": "🟠",
|
||||
"medium": "🟡",
|
||||
"low": "🔵",
|
||||
}.get(severity, "🟡")
|
||||
|
||||
|
||||
def _split_title_and_detail(description: str) -> tuple[str, str]:
|
||||
"""Split a description into a short bold title and the remaining detail.
|
||||
|
||||
The first line becomes the title; everything after it is the detail, so the
|
||||
title text is never duplicated in the body.
|
||||
"""
|
||||
if not description:
|
||||
return "Code review finding", ""
|
||||
lines = description.split("\n")
|
||||
first_line = lines[0].strip()
|
||||
detail = "\n".join(lines[1:]).strip()
|
||||
if len(first_line) > 120:
|
||||
return first_line[:117] + "...", description
|
||||
return first_line, detail
|
||||
|
||||
|
||||
def _format_line_reference(start_line: int | None, end_line: int | None) -> str:
|
||||
"""Format the line reference footer."""
|
||||
if end_line is None:
|
||||
return ""
|
||||
if start_line is None or start_line == end_line:
|
||||
return f"*(Refers to line {end_line})*"
|
||||
return f"*(Refers to lines {start_line}-{end_line})*"
|
||||
|
||||
|
||||
def render_resolution_comment(finding: Finding, status: str, note: str | None = None) -> str:
|
||||
"""Render the comment posted to a review thread when a finding is resolved.
|
||||
|
||||
``note`` (an explanation from the agent's ``update_finding``/resolve call)
|
||||
becomes the body. Falls back to the finding's stored reconciliation note, then
|
||||
to a generic line.
|
||||
"""
|
||||
if note is None:
|
||||
note = finding.get("last_reconciliation_note")
|
||||
note = (note or "").strip()
|
||||
if status == "resolved":
|
||||
body = note or (
|
||||
"The reported issue is no longer present in the current code; "
|
||||
"this finding has been fixed."
|
||||
)
|
||||
return f"✅ **Resolved**: {body}"
|
||||
body = note or "This finding has been dismissed after further review."
|
||||
return f"❌ **Dismissed**: {body}"
|
||||
|
||||
|
||||
def render_inline_comment_payload(finding: Finding) -> dict[str, Any] | None:
|
||||
"""Render one finding into the payload shape GitHub's Reviews API expects.
|
||||
|
||||
|
|
|
|||
|
|
@ -29,7 +29,9 @@ from ..reviewer_publish import (
|
|||
parse_review_comment_marker,
|
||||
post_pull_request_review,
|
||||
render_inline_comment_payload,
|
||||
render_resolution_comment,
|
||||
render_review_body,
|
||||
reply_to_review_comment,
|
||||
resolve_review_thread,
|
||||
)
|
||||
from ..reviewer_reconcile import reconcile_findings_with_review_threads
|
||||
|
|
@ -759,17 +761,21 @@ async def _resolve_threads_for_resolved_findings(
|
|||
) -> int:
|
||||
"""Resolve GitHub review threads for findings that just transitioned to resolved.
|
||||
|
||||
Resolves every known GitHub thread for a finding. Multiple threads can
|
||||
exist when an earlier run duplicated a comment before publication identity
|
||||
Posts a resolution comment to the thread, then resolves it. 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") not in {"resolved", "dismissed"}:
|
||||
status = finding.get("status")
|
||||
if status not in {"resolved", "dismissed"}:
|
||||
continue
|
||||
|
||||
thread_node_ids = _thread_ids_for_finding(finding)
|
||||
for comment_id in _comment_ids_for_finding(finding):
|
||||
comment_ids = _comment_ids_for_finding(finding)
|
||||
|
||||
for comment_id in comment_ids:
|
||||
thread_node_id = await fetch_review_thread_id_for_comment(
|
||||
owner=owner,
|
||||
repo=repo,
|
||||
|
|
@ -784,9 +790,28 @@ async def _resolve_threads_for_resolved_findings(
|
|||
continue
|
||||
|
||||
resolved_thread_ids = _str_list(finding.get("github_resolved_thread_ids"))
|
||||
for thread_node_id in thread_node_ids:
|
||||
posted_resolution_comment_ids = _int_list(
|
||||
finding.get("github_posted_resolution_comment_ids")
|
||||
)
|
||||
|
||||
for idx, thread_node_id in enumerate(thread_node_ids):
|
||||
if thread_node_id in resolved_thread_ids:
|
||||
continue
|
||||
|
||||
primary_comment_id = comment_ids[idx] if idx < len(comment_ids) else None
|
||||
if primary_comment_id and primary_comment_id not in posted_resolution_comment_ids:
|
||||
reply_response = await reply_to_review_comment(
|
||||
owner=owner,
|
||||
repo=repo,
|
||||
pr_number=pr_number,
|
||||
review_comment_id=primary_comment_id,
|
||||
body=render_resolution_comment(finding, status),
|
||||
token=token,
|
||||
)
|
||||
if reply_response and isinstance(reply_response.get("id"), int):
|
||||
posted_resolution_comment_ids.append(primary_comment_id)
|
||||
mutated = True
|
||||
|
||||
ok = await resolve_review_thread(thread_node_id=thread_node_id, token=token)
|
||||
if ok:
|
||||
resolved_thread_ids.append(thread_node_id)
|
||||
|
|
@ -795,6 +820,8 @@ async def _resolve_threads_for_resolved_findings(
|
|||
|
||||
if resolved_thread_ids:
|
||||
finding["github_resolved_thread_ids"] = resolved_thread_ids
|
||||
if posted_resolution_comment_ids:
|
||||
finding["github_posted_resolution_comment_ids"] = posted_resolution_comment_ids
|
||||
if thread_node_ids:
|
||||
finding["github_review_thread_ids"] = thread_node_ids
|
||||
if not isinstance(finding.get("github_review_thread_id"), str):
|
||||
|
|
|
|||
|
|
@ -15,6 +15,8 @@ from ..reviewer_findings import (
|
|||
from ..reviewer_publish import (
|
||||
fetch_pr_review_threads,
|
||||
fetch_review_thread_id_for_comment,
|
||||
render_resolution_comment,
|
||||
reply_to_review_comment,
|
||||
resolve_review_thread,
|
||||
)
|
||||
from ..reviewer_reconcile import reconcile_findings_with_review_threads
|
||||
|
|
@ -101,10 +103,26 @@ async def _resolve_finding_thread_async(
|
|||
return {"success": False, "error": "Could not resolve GitHub review thread id"}
|
||||
|
||||
resolved_thread_ids = _str_list(finding.get("github_resolved_thread_ids"))
|
||||
posted_resolution_comment_ids = _int_list(finding.get("github_posted_resolution_comment_ids"))
|
||||
comment_ids = _comment_ids_for_finding(finding)
|
||||
resolution_body = render_resolution_comment(finding, status, note=note)
|
||||
|
||||
resolved_count = 0
|
||||
for github_thread_id in github_thread_ids:
|
||||
for idx, github_thread_id in enumerate(github_thread_ids):
|
||||
if github_thread_id in resolved_thread_ids:
|
||||
continue
|
||||
primary_comment_id = comment_ids[idx] if idx < len(comment_ids) else None
|
||||
if primary_comment_id and primary_comment_id not in posted_resolution_comment_ids:
|
||||
reply = await reply_to_review_comment(
|
||||
owner=owner,
|
||||
repo=repo,
|
||||
pr_number=pr_number,
|
||||
review_comment_id=primary_comment_id,
|
||||
body=resolution_body,
|
||||
token=token,
|
||||
)
|
||||
if reply and isinstance(reply.get("id"), int):
|
||||
posted_resolution_comment_ids.append(primary_comment_id)
|
||||
ok = await resolve_review_thread(thread_node_id=github_thread_id, token=token)
|
||||
if ok:
|
||||
resolved_thread_ids.append(github_thread_id)
|
||||
|
|
@ -125,6 +143,8 @@ async def _resolve_finding_thread_async(
|
|||
}
|
||||
if note:
|
||||
updates["last_reconciliation_note"] = note
|
||||
if posted_resolution_comment_ids:
|
||||
updates["github_posted_resolution_comment_ids"] = posted_resolution_comment_ids
|
||||
updated = await update_finding_fields(thread_id, finding_id, updates)
|
||||
surface_updates: dict[str, Any] = {
|
||||
"state": "resolved" if updates["github_thread_resolved"] else "resolve_pending",
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ from agent.reviewer_publish import (
|
|||
post_pull_request_review,
|
||||
render_inline_comment_body,
|
||||
render_inline_comment_payload,
|
||||
render_resolution_comment,
|
||||
render_review_body,
|
||||
reply_to_review_comment,
|
||||
resolve_review_thread,
|
||||
|
|
@ -50,7 +51,8 @@ def test_render_inline_comment_body_without_suggestion() -> None:
|
|||
assert "<!-- open-swe-review-comment" in body
|
||||
assert '"id":"f_' in body
|
||||
assert "just text" in body
|
||||
assert "React with +1 or -1" not in body
|
||||
assert "Was this helpful?" in body
|
||||
assert "👍 or 👎" in body
|
||||
|
||||
|
||||
def test_render_inline_comment_body_with_suggestion_appends_block() -> None:
|
||||
|
|
@ -62,6 +64,55 @@ def test_render_inline_comment_body_with_suggestion_appends_block() -> None:
|
|||
assert "x = 1\nx += 1" in body
|
||||
|
||||
|
||||
def test_render_inline_comment_body_uses_severity_emoji_and_bold_title() -> None:
|
||||
body = render_inline_comment_body(_f(severity="critical", description="Null deref"))
|
||||
assert "🔴 **Null deref**" in body
|
||||
|
||||
|
||||
def test_render_inline_comment_body_does_not_duplicate_first_line() -> None:
|
||||
body = render_inline_comment_body(
|
||||
_f(description="Short summary line\n\nLonger detail paragraph."),
|
||||
)
|
||||
assert "**Short summary line**" in body
|
||||
assert "Longer detail paragraph." in body
|
||||
# The first line is the bold title and must not also appear in the detail body.
|
||||
assert body.count("Short summary line") == 1
|
||||
|
||||
|
||||
def test_render_inline_comment_body_single_line_has_no_detail() -> None:
|
||||
body = render_inline_comment_body(_f(description="just text"))
|
||||
assert body.count("just text") == 1
|
||||
|
||||
|
||||
def test_render_inline_comment_body_line_reference_range() -> None:
|
||||
assert "*(Refers to lines 10-12)*" in render_inline_comment_body(_f(start_line=10, end_line=12))
|
||||
assert "*(Refers to line 10)*" in render_inline_comment_body(_f(start_line=10, end_line=10))
|
||||
|
||||
|
||||
def test_render_resolution_comment_resolved_uses_note() -> None:
|
||||
body = render_resolution_comment(_f(status="resolved"), "resolved", note="Fixed at line 5")
|
||||
assert body == "✅ **Resolved**: Fixed at line 5"
|
||||
|
||||
|
||||
def test_render_resolution_comment_resolved_falls_back_without_note() -> None:
|
||||
body = render_resolution_comment(_f(status="resolved"), "resolved")
|
||||
assert body.startswith("✅ **Resolved**:")
|
||||
assert "no longer present" in body
|
||||
|
||||
|
||||
def test_render_resolution_comment_dismissed_uses_note() -> None:
|
||||
body = render_resolution_comment(_f(status="dismissed"), "dismissed", note="Intended behavior")
|
||||
assert body == "❌ **Dismissed**: Intended behavior"
|
||||
|
||||
|
||||
def test_render_resolution_comment_handles_none_reconciliation_note() -> None:
|
||||
# Regression: last_reconciliation_note defaults to None on a fresh finding.
|
||||
finding = _f(status="resolved")
|
||||
assert finding.get("last_reconciliation_note") is None
|
||||
body = render_resolution_comment(finding, "resolved")
|
||||
assert body.startswith("✅ **Resolved**:")
|
||||
|
||||
|
||||
def test_parse_review_comment_marker_accepts_valid_marker() -> None:
|
||||
finding = _f(
|
||||
id="f_marker",
|
||||
|
|
@ -448,6 +499,7 @@ async def test_re_review_backfills_and_resolves_duplicate_existing_threads() ->
|
|||
},
|
||||
]
|
||||
resolve_thread = AsyncMock(return_value=True)
|
||||
reply_comment = AsyncMock(return_value={"id": 555})
|
||||
|
||||
with (
|
||||
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
|
||||
|
|
@ -459,6 +511,11 @@ async def test_re_review_backfills_and_resolves_duplicate_existing_threads() ->
|
|||
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.fetch_review_thread_id_for_comment",
|
||||
AsyncMock(return_value=None),
|
||||
),
|
||||
patch("agent.tools.publish_review.reply_to_review_comment", reply_comment),
|
||||
patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock),
|
||||
):
|
||||
result = await _publish_review_async(
|
||||
|
|
@ -476,9 +533,12 @@ async def test_re_review_backfills_and_resolves_duplicate_existing_threads() ->
|
|||
assert result["review_id"] is None
|
||||
assert result["resolved_thread_count"] == 2
|
||||
assert resolve_thread.await_count == 2
|
||||
assert reply_comment.await_count == 2
|
||||
assert "✅ **Resolved**" in reply_comment.await_args_list[0].kwargs["body"]
|
||||
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_posted_resolution_comment_ids"] == [101, 102]
|
||||
assert findings[0]["github_thread_resolved"] is True
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -212,6 +212,7 @@ def test_resolve_finding_thread_resolves_all_known_threads() -> None:
|
|||
}
|
||||
update = AsyncMock(return_value={**finding, "status": "resolved"})
|
||||
resolve = AsyncMock(return_value=True)
|
||||
reply = AsyncMock(return_value={"id": 999})
|
||||
|
||||
with (
|
||||
patch(
|
||||
|
|
@ -222,9 +223,10 @@ def test_resolve_finding_thread_resolves_all_known_threads() -> None:
|
|||
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.reply_to_review_comment", reply),
|
||||
patch("agent.tools.resolve_finding_thread.update_finding_fields", update),
|
||||
):
|
||||
result = resolve_finding_thread("f1", status="resolved")
|
||||
result = resolve_finding_thread("f1", status="resolved", note="Fixed in the latest commit")
|
||||
|
||||
assert result["success"] is True
|
||||
assert result["resolved_thread_count"] == 2
|
||||
|
|
@ -232,9 +234,15 @@ def test_resolve_finding_thread_resolves_all_known_threads() -> None:
|
|||
"THREAD_1",
|
||||
"THREAD_2",
|
||||
]
|
||||
assert [call.kwargs["review_comment_id"] for call in reply.await_args_list] == [11, 12]
|
||||
assert all(
|
||||
"✅ **Resolved**: Fixed in the latest commit" in call.kwargs["body"]
|
||||
for call in reply.await_args_list
|
||||
)
|
||||
updates = update.await_args.args[2]
|
||||
assert updates["github_thread_resolved"] is True
|
||||
assert updates["github_resolved_thread_ids"] == ["THREAD_1", "THREAD_2"]
|
||||
assert updates["github_posted_resolution_comment_ids"] == [11, 12]
|
||||
|
||||
|
||||
def test_update_finding_rejects_empty_update() -> None:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue