feat: let reviewer set comment titles (#1356)

Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com>

agent/reviewer.py

@@ -98,7 +98,7 @@ Tools: `add_finding`, `update_finding`, `list_findings`, `publish_review`,
 Call `publish_review` once at the end.
 
 If `publish_review` returns `unresolvable_findings`, do NOT retry with the
-same args — call `update_finding(status="resolved")` on those ids, or fix
+same args — call `update_finding(status="resolved", note="...")` 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", note="...")`
@@ -367,11 +367,11 @@ def _build_re_review_context(
         f'{last_reviewed_sha}...{head_sha} -H "Accept: application/vnd.github.v3.diff"`, '
         f"then review only what's in that diff.\n\n"
         f"For each open finding above, decide whether the new commits resolved "
-        f'it (`update_finding(id, status="resolved")`), left it unchanged '
+        f'it (`update_finding(id, status="resolved", note="...")`), left it unchanged '
         f"(no action), or changed it materially (`update_finding` with new "
         f"fields + a `note`). If a human reply on a finding explains why your "
         f"comment was invalid, verify that analysis, then call "
-        f'`resolve_finding_thread(id, status="dismissed")` to close it. '
+        f'`resolve_finding_thread(id, status="dismissed", note="...")` to close it. '
         f"Reply only when directly asked or when a concise clarification is "
         f"necessary. Then add any net-new findings introduced by the "
         f"new diff — but skip anything already covered by an existing PR "
@@ -418,8 +418,8 @@ def _build_finding_reply_context(
         f"## Existing findings\n\n{existing_findings_block}\n\n"
         f"{prior_threads_section}"
         f"Reassess only this finding. If the reply proves the finding is invalid, "
-        f'call `resolve_finding_thread(id, status="dismissed")`. If code now '
+        f'call `resolve_finding_thread(id, status="dismissed", note="...")`. If code now '
-        f'fixes the finding, call `update_finding(id, status="resolved")`. '
+        f'fixes the finding, call `update_finding(id, status="resolved", note="...")`. '
         f"Use `reply_to_finding_thread` only when the user asked a direct "
         f"question or a concise clarification is necessary. Call `publish_review` "
         f"once at the end so pending GitHub thread state is reconciled."

agent/reviewer_findings.py

@@ -107,6 +107,7 @@ class Finding(TypedDict, total=False):
     last_human_reply_author: str | None
     last_human_reply_body: str | None
     last_reconciliation_note: str | None
+    resolution_note: str | None
     diff_hunk: str | None
     fingerprint: str
     anchor: FindingAnchor
@@ -229,6 +230,7 @@ def new_finding(
         "last_human_reply_author": None,
         "last_human_reply_body": None,
         "last_reconciliation_note": None,
+        "resolution_note": None,
         "diff_hunk": diff_hunk,
         "fingerprint": _finding_fingerprint(file, start_line, end_line, description),
         "anchor": anchor,

agent/reviewer_publish.py

@@ -175,26 +175,28 @@ def _format_line_reference(start_line: int | None, end_line: int | None) -> str:
     return f"*(Refers to lines {start_line}-{end_line})*"
 
 
-def render_resolution_comment(finding: Finding, status: str, note: str | None = None) -> str:
+def render_resolution_comment(
-    """Render the comment posted to a review thread when a finding is resolved.
+    finding: Finding,
-
+    status: str,
-    ``note`` (an explanation from the agent's ``update_finding``/resolve call)
+    note: str | None = None,
-    becomes the body. Falls back to the finding's stored reconciliation note, then
+) -> str | None:
-    to a generic line.
+    """Render the agent-provided resolution reply for a review thread."""
-    """
+    body = _resolution_body(finding, note)
-    if note is None:
+    if body is None:
-        note = finding.get("last_reconciliation_note")
+        return None
-    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 _resolution_body(finding: Finding, note: str | None) -> str | None:
+    candidates = [note, finding.get("resolution_note"), finding.get("last_update_note")]
+    for candidate in candidates:
+        if isinstance(candidate, str) and candidate.strip():
+            return candidate.strip()
+    return None
+
+
 def render_inline_comment_payload(finding: Finding) -> dict[str, Any] | None:
     """Render one finding into the payload shape GitHub's Reviews API expects.
 

agent/reviewer_reconcile.py

@@ -165,7 +165,6 @@ def _sync_thread_status(finding: Finding, matches: list[ReviewThreadMatch]) -> b
     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"))

agent/tools/publish_review.py

@@ -820,13 +820,18 @@ async def _resolve_threads_for_resolved_findings(
                 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:
+            resolution_body = render_resolution_comment(finding, status)
+            if (
+                primary_comment_id
+                and primary_comment_id not in posted_resolution_comment_ids
+                and resolution_body is not None
+            ):
                 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),
+                    body=resolution_body,
                     token=token,
                 )
                 if reply_response and isinstance(reply_response.get("id"), int):

agent/tools/resolve_finding_thread.py

@@ -23,19 +23,32 @@ from ..reviewer_reconcile import reconcile_findings_with_review_threads
 from ..utils.github_token import get_github_token
 
 
+def _normalize_note(note: str | None) -> str | None:
+    if note is None:
+        return None
+    normalized = note.strip()
+    return normalized or None
+
+
 def resolve_finding_thread(
     finding_id: str,
+    note: str,
     status: str = "dismissed",
-    note: str | None = None,
 ) -> dict[str, Any]:
     """Resolve the GitHub review thread for a tracked Open SWE finding.
 
     Use ``status="resolved"`` when the code now fixes the issue. Use
     ``status="dismissed"`` when analysis shows the original review comment was
-    not valid.
+    not valid. ``note`` is required and becomes the GitHub reply body.
     """
     if status not in {"resolved", "dismissed"}:
         return {"success": False, "error": f"Invalid status: {status}"}
+    normalized_note = _normalize_note(note)
+    if normalized_note is None:
+        return {
+            "success": False,
+            "error": "Resolving or dismissing a finding requires a note with the message to post.",
+        }
 
     config = get_config()
     configurable = config.get("configurable", {}) if isinstance(config, dict) else {}
@@ -57,7 +70,7 @@ def resolve_finding_thread(
         _resolve_finding_thread_async(
             finding_id=finding_id,
             status=status,
-            note=note,
+            note=normalized_note,
             owner=str(repo_config["owner"]),
             repo=str(repo_config["name"]),
             pr_number=pr_number,
@@ -70,7 +83,7 @@ async def _resolve_finding_thread_async(
     *,
     finding_id: str,
     status: str,
-    note: str | None,
+    note: str,
     owner: str,
     repo: str,
     pr_number: int,
@@ -106,6 +119,8 @@ async def _resolve_finding_thread_async(
     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)
+    if resolution_body is None:
+        return {"success": False, "error": "Missing resolution note"}
 
     resolved_count = 0
     for idx, github_thread_id in enumerate(github_thread_ids):
@@ -141,8 +156,8 @@ async def _resolve_finding_thread_async(
             github_thread_id in resolved_thread_ids for github_thread_id in github_thread_ids
         ),
     }
-    if note:
+    updates["last_reconciliation_note"] = note
-        updates["last_reconciliation_note"] = note
+    updates["resolution_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)

agent/tools/update_finding.py

@@ -23,6 +23,13 @@ def _is_non_empty_str(value: Any) -> bool:
     return isinstance(value, str) and bool(value)
 
 
+def _normalize_note(note: str | None) -> str | None:
+    if note is None:
+        return None
+    normalized = note.strip()
+    return normalized or None
+
+
 def _has_published_github_surface(finding: Finding) -> bool:
     surface = finding.get("surface")
     if isinstance(surface, dict) and (
@@ -60,7 +67,8 @@ def update_finding(
         finding_id: The id returned by ``add_finding`` (or shown in the
             ``Existing findings`` block of the re-review user message).
         status: New status (``open``, ``resolved``, ``dismissed``).
-            Use ``resolved`` when the new commits address the issue.
+            Use ``resolved`` when the new commits address the issue. Resolving
+            or dismissing requires a ``note`` with the message to post.
         severity: New severity, if reassessing.
         confidence: New confidence rating (``low``, ``medium``, ``high``), if
             new commits change how sure you are the finding is a real issue.
@@ -70,8 +78,8 @@ def update_finding(
         suggestion: New replacement text. Pass an empty string to clear it.
             Capped at 4 lines — longer values are dropped (the finding keeps
             its description). Only set this for small, obvious fixes.
-        note: Optional free-form note explaining the change. Persisted on the
+        note: Optional free-form note explaining the change. Required when
-            finding under ``last_update_note``.
+            resolving or dismissing because it becomes the GitHub reply body.
 
     Returns:
         Dictionary with ``success`` and (on success) the updated ``finding``.
@@ -82,6 +90,12 @@ def update_finding(
         return {"success": False, "error": f"Invalid severity: {severity}"}
     if confidence is not None and confidence not in {"low", "medium", "high"}:
         return {"success": False, "error": f"Invalid confidence: {confidence}"}
+    normalized_note = _normalize_note(note)
+    if status in {"resolved", "dismissed"} and normalized_note is None:
+        return {
+            "success": False,
+            "error": "Resolving or dismissing a finding requires a note with the message to post.",
+        }
 
     updates: dict[str, Any] = {}
     suggestion_dropped = False
@@ -105,8 +119,10 @@ def update_finding(
             clipped, suggestion_dropped = clip_suggestion(suggestion)
             if not suggestion_dropped:
                 updates["suggestion"] = clipped
-    if note is not None:
+    if normalized_note is not None:
-        updates["last_update_note"] = note
+        updates["last_update_note"] = normalized_note
+        if status in {"resolved", "dismissed"}:
+            updates["resolution_note"] = normalized_note
 
     config = get_config()
     configurable = config.get("configurable", {}) if isinstance(config, dict) else {}
@@ -149,7 +165,7 @@ def update_finding(
     ):
         from .resolve_finding_thread import resolve_finding_thread
 
-        resolve_result = resolve_finding_thread(finding_id, status=status, note=note)
+        resolve_result = resolve_finding_thread(finding_id, status=status, note=normalized_note)
         if not resolve_result.get("success"):
             return {
                 "success": False,
@@ -158,6 +174,7 @@ def update_finding(
             }
         updates.pop("status", None)
         updates.pop("last_update_note", None)
+        updates.pop("resolution_note", None)
         if not updates:
             result: dict[str, Any] = {
                 "success": True,

tests/test_reviewer_findings.py

@@ -57,6 +57,7 @@ def test_new_finding_defaults() -> None:
     assert finding["github_thread_resolved"] is False
     assert finding["github_resolved_thread_ids"] == []
     assert finding["last_human_reply_at"] is None
+    assert finding["resolution_note"] is None
     assert finding["suggestion"] is None
 
 

tests/test_reviewer_publish.py

@@ -113,10 +113,9 @@ def test_render_resolution_comment_resolved_uses_note() -> None:
     assert body == "✅ **Resolved**: Fixed at line 5"
 
 
-def test_render_resolution_comment_resolved_falls_back_without_note() -> None:
+def test_render_resolution_comment_returns_none_without_agent_note() -> None:
     body = render_resolution_comment(_f(status="resolved"), "resolved")
-    assert body.startswith("✅ **Resolved**:")
+    assert body is None
-    assert "no longer present" in body
 
 
 def test_render_resolution_comment_dismissed_uses_note() -> None:
@@ -124,12 +123,10 @@ def test_render_resolution_comment_dismissed_uses_note() -> None:
     assert body == "❌ **Dismissed**: Intended behavior"
 
 
-def test_render_resolution_comment_handles_none_reconciliation_note() -> None:
+def test_render_resolution_comment_uses_stored_resolution_note() -> None:
-    # Regression: last_reconciliation_note defaults to None on a fresh finding.
+    finding = _f(status="resolved", resolution_note="The guard now returns before indexing.")
-    finding = _f(status="resolved")
-    assert finding.get("last_reconciliation_note") is None
     body = render_resolution_comment(finding, "resolved")
-    assert body.startswith("✅ **Resolved**:")
+    assert body == "✅ **Resolved**: The guard now returns before indexing."
 
 
 def test_parse_review_comment_marker_accepts_valid_marker() -> None:
@@ -578,6 +575,7 @@ async def test_re_review_backfills_and_resolves_duplicate_existing_threads() ->
         first_seen_sha="oldsha",
         github_review_comment_id=None,
         status="resolved",
+        resolution_note="The duplicate threads are fixed by the latest commit.",
     )
     findings = [finding]
     threads = [
@@ -644,7 +642,10 @@ async def test_re_review_backfills_and_resolves_duplicate_existing_threads() ->
     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 (
+        reply_comment.await_args_list[0].kwargs["body"]
+        == "✅ **Resolved**: The duplicate threads are fixed by the latest commit."
+    )
     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"]

tests/test_reviewer_reconcile.py

@@ -213,7 +213,7 @@ async def test_reconcile_duplicate_markers_resolve_when_all_threads_terminal() -
         )
 
     assert result[0]["status"] == "resolved"
-    assert result[0]["last_reconciliation_note"] == "All GitHub threads are resolved or outdated."
+    assert "last_reconciliation_note" not in result[0]
     assert result[0]["github_resolved_thread_ids"] == ["THREAD_RESOLVED"]
     assert result[0].get("github_thread_resolved") is not True
     replace.assert_awaited_once()

tests/test_reviewer_tools.py

@@ -267,6 +267,17 @@ def test_resolve_finding_thread_resolves_all_known_threads() -> None:
     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]
+    assert updates["resolution_note"] == "Fixed in the latest commit"
+
+
+def test_resolve_finding_thread_requires_note() -> None:
+    with patch(
+        "agent.tools.resolve_finding_thread.get_config",
+        return_value=_config(repo={"owner": "o", "name": "r"}, pr_number=7),
+    ):
+        result = resolve_finding_thread("f1", note=" ", status="resolved")
+    assert result["success"] is False
+    assert "requires a note" in result["error"]
 
 
 def test_update_finding_rejects_empty_update() -> None:
@@ -276,6 +287,13 @@ def test_update_finding_rejects_empty_update() -> None:
     assert "No fields" in result["error"]
 
 
+def test_update_finding_requires_note_for_resolution() -> None:
+    with patch("agent.tools.update_finding.get_config", return_value=_config()):
+        result = update_finding(finding_id="f_x", status="resolved")
+    assert result["success"] is False
+    assert "requires a note" in result["error"]
+
+
 def test_update_finding_updates_title() -> None:
     captured: list[Any] = []
 
@@ -481,6 +499,7 @@ def test_update_finding_passes_through_fields() -> None:
     assert fid == "f_a"
     assert updates["status"] == "resolved"
     assert updates["last_update_note"] == "addressed by new commit"
+    assert updates["resolution_note"] == "addressed by new commit"
 
 
 def test_update_finding_resolves_github_thread_when_pr_context_available() -> None:
@@ -505,7 +524,11 @@ def test_update_finding_resolves_github_thread_when_pr_context_available() -> No
             },
         ) as resolve_async,
     ):
-        result = update_finding(finding_id="f_a", status="resolved")
+        result = update_finding(
+            finding_id="f_a",
+            status="resolved",
+            note="The latest commit adds the missing guard.",
+        )
 
     assert result["success"] is True
     assert result["github_resolution"]["success"] is True
@@ -535,7 +558,11 @@ def test_update_finding_leaves_open_when_github_resolution_fails() -> None:
             },
         ) as resolve_async,
     ):
-        result = update_finding(finding_id="f_a", status="resolved")
+        result = update_finding(
+            finding_id="f_a",
+            status="resolved",
+            note="The latest commit adds the missing guard.",
+        )
 
     assert result["success"] is False
     assert "left open" in result["error"]
@@ -565,11 +592,16 @@ def test_update_finding_resolves_hidden_finding_locally() -> None:
             new_callable=AsyncMock,
         ) as resolve_async,
     ):
-        result = update_finding(finding_id="f_a", status="resolved")
+        result = update_finding(
+            finding_id="f_a",
+            status="resolved",
+            note="The latest commit adds the missing guard.",
+        )
 
     assert result["success"] is True
     _thread_id, _finding_id, updates = captured[0]
     assert updates["status"] == "resolved"
+    assert updates["resolution_note"] == "The latest commit adds the missing guard."
     resolve_async.assert_not_awaited()