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: - + + + 🟡 **Title (first line of the description)** + + + + *(Refers to lines X-Y)* + + --- + *Was this helpful? React with 👍 or 👎 to provide feedback.* ```suggestion @@ -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"" + + 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 "