From fee209601b04c7c3592ce770d022759ae88f21ee Mon Sep 17 00:00:00 2001
From: Johannes du Plessis <johannes@langchain.dev>
Date: Thu, 28 May 2026 12:44:00 -0700
Subject: [PATCH] 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.
---
 agent/reviewer.py                     | 24 +++++---
 agent/reviewer_findings.py            |  2 +
 agent/reviewer_publish.py             | 81 ++++++++++++++++++++++++++-
 agent/tools/publish_review.py         | 37 ++++++++++--
 agent/tools/resolve_finding_thread.py | 22 +++++++-
 tests/test_reviewer_publish.py        | 62 +++++++++++++++++++-
 tests/test_reviewer_tools.py          | 10 +++-
 7 files changed, 218 insertions(+), 20 deletions(-)

diff --git a/agent/reviewer.py b/agent/reviewer.py
index 55c67a78..0905ed81 100644
--- a/agent/reviewer.py
+++ b/agent/reviewer.py
@@ -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
 
diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py
index 7224f76f..59bb9e9b 100644
--- a/agent/reviewer_findings.py
+++ b/agent/reviewer_findings.py
@@ -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,
diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py
index 25db1715..4b508f6d 100644
--- a/agent/reviewer_publish.py
+++ b/agent/reviewer_publish.py
@@ -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.
 
diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py
index af244c72..eb1381ca 100644
--- a/agent/tools/publish_review.py
+++ b/agent/tools/publish_review.py
@@ -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):
diff --git a/agent/tools/resolve_finding_thread.py b/agent/tools/resolve_finding_thread.py
index 28b5c57a..1f07fa03 100644
--- a/agent/tools/resolve_finding_thread.py
+++ b/agent/tools/resolve_finding_thread.py
@@ -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",
diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py
index dd14c23f..6eeb3007 100644
--- a/tests/test_reviewer_publish.py
+++ b/tests/test_reviewer_publish.py
@@ -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
 
 
diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py
index a0d7d17a..2ceb06cd 100644
--- a/tests/test_reviewer_tools.py
+++ b/tests/test_reviewer_tools.py
@@ -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: