diff --git a/agent/reviewer.py b/agent/reviewer.py index 007a8aa0..bbfe24a9 100644 --- a/agent/reviewer.py +++ b/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'fixes the finding, call `update_finding(id, status="resolved")`. ' + f'call `resolve_finding_thread(id, status="dismissed", note="...")`. If code now ' + 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." diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index 94866b82..8972b7d3 100644 --- a/agent/reviewer_findings.py +++ b/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, diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 06550288..806e17c7 100644 --- a/agent/reviewer_publish.py +++ b/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: - """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() +def render_resolution_comment( + finding: Finding, + status: str, + note: str | None = None, +) -> str | None: + """Render the agent-provided resolution reply for a review thread.""" + body = _resolution_body(finding, note) + if body is None: + return None 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. diff --git a/agent/reviewer_reconcile.py b/agent/reviewer_reconcile.py index d9659ace..f43e93eb 100644 --- a/agent/reviewer_reconcile.py +++ b/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")) diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 6dce08aa..23c78bb0 100644 --- a/agent/tools/publish_review.py +++ b/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): diff --git a/agent/tools/resolve_finding_thread.py b/agent/tools/resolve_finding_thread.py index 1f07fa03..aed1927d 100644 --- a/agent/tools/resolve_finding_thread.py +++ b/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) diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py index ce03b8e6..ad02f8b2 100644 --- a/agent/tools/update_finding.py +++ b/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 - finding under ``last_update_note``. + note: Optional free-form note explaining the change. Required when + 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: - updates["last_update_note"] = note + if normalized_note is not None: + 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, diff --git a/tests/test_reviewer_findings.py b/tests/test_reviewer_findings.py index 5e3c00df..30c04bc1 100644 --- a/tests/test_reviewer_findings.py +++ b/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 diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index f4d4369b..21c2916b 100644 --- a/tests/test_reviewer_publish.py +++ b/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 "no longer present" in body + assert body is None 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: - # Regression: last_reconciliation_note defaults to None on a fresh finding. - finding = _f(status="resolved") - assert finding.get("last_reconciliation_note") is None +def test_render_resolution_comment_uses_stored_resolution_note() -> None: + finding = _f(status="resolved", resolution_note="The guard now returns before indexing.") 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"] diff --git a/tests/test_reviewer_reconcile.py b/tests/test_reviewer_reconcile.py index fe61df4c..3e8b1180 100644 --- a/tests/test_reviewer_reconcile.py +++ b/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() diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index b2a08555..45d6a07e 100644 --- a/tests/test_reviewer_tools.py +++ b/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()