diff --git a/agent/reviewer.py b/agent/reviewer.py index 9afcc0d2..5c224652 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -102,6 +102,12 @@ started — you do **not** need to clone, fetch, or check out yourself. once** at the end of the run. It batches eligible findings into a single GitHub PR Review with inline comments + suggestion blocks, and stores the GitHub comment IDs back so re-reviews can later resolve threads. + - Always pass a `summary` — a 1–2 sentence top-level take that's posted + as the review body above any inline comments. When you found no issues, + write a short summary that confirms the PR was reviewed and notes + anything worth calling out (test coverage, scope, follow-ups). Don't + skip the summary just because you have nothing critical to flag — the + summary is how the user knows the review ran. ### Re-reviewing on a new commit diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 2bda2427..51fc6a21 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -93,18 +93,25 @@ def render_review_body( when findings were filtered out below the surfacing threshold. """ parts: list[str] = [] - if summary: - parts.append(summary.strip()) - if surfaced_count == 0: - parts.append("_No issues at or above the configured severity threshold._") + if surfaced_count == 0 and total_open_count == 0: + parts.append("**No issues found.**") + elif surfaced_count == 0: + parts.append( + f"**No issues at or above `{severity_threshold}` severity.** " + f"({total_open_count} lower-severity finding" + f"{'s' if total_open_count != 1 else ''} hidden.)" + ) else: + finding_word = "finding" if surfaced_count == 1 else "findings" + parts.append(f"**Found {surfaced_count} {finding_word}.**") hidden = total_open_count - surfaced_count if hidden > 0: parts.append( - f"_Showing {surfaced_count} finding{'s' if surfaced_count != 1 else ''} " - f"at severity ≥ `{severity_threshold}`; {hidden} lower-severity " - f"finding{'s' if hidden != 1 else ''} hidden._" + f"_{hidden} lower-severity finding{'s' if hidden != 1 else ''} " + f"below `{severity_threshold}` hidden._" ) + if summary: + parts.append(summary.strip()) parts.append(f"") return "\n\n".join(p for p in parts if p) diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 422d897f..783dbd82 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -150,20 +150,18 @@ async def _publish_review_async( summary=summary, ) - review_id: int | None = None - if inline_comments or summary: - review_response = await post_pull_request_review( - owner=owner, - repo=repo, - pr_number=pr_number, - head_sha=head_sha, - body=review_body, - inline_comments=inline_comments, - token=token, - ) - if review_response is None: - return {"success": False, "error": "Failed to POST PR review"} - review_id = review_response.get("id") if isinstance(review_response, dict) else None + review_response = await post_pull_request_review( + owner=owner, + repo=repo, + pr_number=pr_number, + head_sha=head_sha, + body=review_body, + inline_comments=inline_comments, + token=token, + ) + if review_response is None: + return {"success": False, "error": "Failed to POST PR review"} + review_id = review_response.get("id") if isinstance(review_response, dict) else None if review_id is not None and inline_comments: comment_records = await fetch_review_comments( diff --git a/agent/webapp.py b/agent/webapp.py index 5ec64b78..84d37eaf 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -1640,6 +1640,12 @@ async def process_github_pr_close(payload: dict[str, Any]) -> None: metadata = await _get_thread_metadata_safe(thread_id) if metadata is None or metadata.get("kind") != REVIEWER_THREAD_KIND: # No reviewer thread for this PR, nothing to do. + logger.debug( + "PR %s/%s#%s closed/reopened: no reviewer thread, skipping watch update", + repo_config.get("owner"), + repo_config.get("name"), + pr_number, + ) return action = payload.get("action", "") desired_watch = action == "reopened" @@ -1654,9 +1660,10 @@ async def process_github_push_event(payload: dict[str, Any]) -> None: ref = payload.get("ref", "") after_sha = payload.get("after", "") if not ref.startswith("refs/heads/"): + logger.debug("Push ignored: ref %s is not a branch", ref) return if not isinstance(after_sha, str) or not after_sha or set(after_sha) == {"0"}: - # Branch deletion or missing SHA — nothing to review. + logger.debug("Push to %s ignored: branch deletion or missing SHA", ref) return head_ref = ref[len("refs/heads/") :] @@ -1666,8 +1673,15 @@ async def process_github_push_event(payload: dict[str, Any]) -> None: "name": repo.get("name", ""), } if not repo_config["owner"] or not repo_config["name"]: + logger.warning("Push to %s ignored: repository owner/name missing from payload", head_ref) return if not _is_repo_allowed_for_reviewer(repo_config): + logger.info( + "Push to %s/%s head=%s ignored: repo not in reviewer allowlist", + repo_config["owner"], + repo_config["name"], + head_ref, + ) return app_token = await get_github_app_installation_token() @@ -1692,11 +1706,25 @@ async def process_github_push_event(payload: dict[str, Any]) -> None: head_sha = pr.get("head", {}).get("sha", after_sha) pr_title = pr.get("title", "") if not isinstance(pr_number, int) or not base_sha or not head_sha: + logger.warning( + "Push to %s/%s head=%s ignored: PR metadata missing number/base/head SHA", + repo_config["owner"], + repo_config["name"], + head_ref, + ) return thread_id = generate_reviewer_thread_id(repo_config["owner"], repo_config["name"], pr_number) metadata = await _get_thread_metadata_safe(thread_id) if metadata is None or metadata.get("kind") != REVIEWER_THREAD_KIND: + logger.info( + "Push to %s/%s#%s ignored: no reviewer thread for this PR. " + "Trigger a first review (Slack `@open-swe review ` or request " + "open-swe[bot] as a GitHub reviewer) to start watching.", + repo_config["owner"], + repo_config["name"], + pr_number, + ) return if not metadata.get("watch"): logger.info("Push to %s ignored: reviewer thread %s is not watching", head_ref, thread_id) diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 7b3d5683..beeaf911 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -77,7 +77,8 @@ def test_render_review_body_includes_summary_and_marker() -> None: ) assert "LGTM with two notes" in body assert "" in body - assert "1 lower-severity finding hidden" in body + assert "Found 2 findings" in body + assert "1 lower-severity finding" in body def test_render_review_body_surfaces_no_findings_message() -> None: @@ -88,7 +89,32 @@ def test_render_review_body_surfaces_no_findings_message() -> None: severity_threshold="medium", summary=None, ) - assert "No issues at or above" in body + assert "No issues found" in body + assert "" in body + + +def test_render_review_body_no_findings_keeps_agent_summary() -> None: + body = render_review_body( + pr_number=42, + surfaced_count=0, + total_open_count=0, + severity_threshold="medium", + summary="Reviewed PR — clean refactor, well-tested.", + ) + assert "No issues found" in body + assert "Reviewed PR — clean refactor, well-tested." in body + + +def test_render_review_body_no_surfaced_with_hidden_lower_severity() -> None: + body = render_review_body( + pr_number=7, + surfaced_count=0, + total_open_count=2, + severity_threshold="medium", + summary=None, + ) + assert "No issues at or above `medium` severity" in body + assert "2 lower-severity findings hidden" in body @pytest.mark.asyncio @@ -166,3 +192,46 @@ async def test_publish_review_skips_findings_already_published() -> None: posted = post_review.await_args.kwargs["inline_comments"] paths = {c["path"] for c in posted} assert paths == {"b.py"} + + +@pytest.mark.asyncio +async def test_publish_review_posts_summary_when_no_findings() -> None: + """An empty findings list must still post a review so the user sees feedback.""" + from agent.tools.publish_review import _publish_review_async + + list_async = AsyncMock(return_value=[]) + post_review = AsyncMock(return_value={"id": 555}) + fetch_comments = AsyncMock(return_value=[]) + set_metadata = AsyncMock() + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", list_async), + patch("agent.tools.publish_review.post_pull_request_review", post_review), + patch("agent.tools.publish_review.fetch_review_comments", fetch_comments), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + summary=None, + severity_threshold="medium", + cap=15, + ) + + assert result["success"] is True + assert result["surfaced_count"] == 0 + assert result["review_id"] == 555 + post_review.assert_awaited_once() + posted_body = post_review.await_args.kwargs["body"] + posted_inline = post_review.await_args.kwargs["inline_comments"] + assert posted_inline == [] + assert "No issues found" in posted_body