From 88a55057ea85d6447443f923346a8d9eecbffa80 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 7 May 2026 15:41:40 -0700 Subject: [PATCH] fix(open-swe): host-format the review summary, drop agent prose (#1257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer agent was writing a 1–2 sentence "top-level take" as the review body, which produced noisy paragraph-style summaries on PRs ("Reviewed the PR. The new ALLOWED_GITHUB_REPOS allowlist…"). Devin's review comment is just a one-liner (`✅ No Issues Found` or `**Devin Review** found N potential issue.`), and was preferred in the internal A/B vs Graphite. Drop the `summary` parameter from `publish_review`; render a fixed, host-formatted body in `render_review_body` instead. Update the reviewer prompt to forbid prose summaries. --- agent/reviewer.py | 10 +++--- agent/reviewer_publish.py | 45 ++++++++------------------ agent/tools/publish_review.py | 12 ++----- tests/test_reviewer_publish.py | 58 +++++++--------------------------- 4 files changed, 32 insertions(+), 93 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index 5c224652..e5403e71 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -102,12 +102,10 @@ 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. + - Do **not** write a summary or top-level take — `publish_review` formats + the review body itself. Your only job is to record findings (or none) + and call the tool. Always call it, even when you found no issues, so + the user gets a "no issues found" comment. ### Re-reviewing on a new commit diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 51fc6a21..f0222a13 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -4,7 +4,8 @@ The reviewer agent calls ``publish_review`` at the end of a run. That tool batches eligible findings (severity ≥ threshold, status=open, capped) into a single GitHub PR Review: -- Review body: agent-authored summary line. +- Review body: a fixed, host-formatted summary line. The agent never writes + prose here — it's either "no issues found" or "found N potential issue(s)". - Inline comments: one per surfaced finding, anchored to ``path`` + ``line`` (+ ``start_line`` for ranges) + ``side``. - Suggestion: when ``finding.suggestion`` is set, appended to the comment body @@ -79,41 +80,23 @@ def render_inline_comment_payload(finding: Finding) -> dict[str, Any] | None: return payload -def render_review_body( - *, - pr_number: int, - surfaced_count: int, - total_open_count: int, - severity_threshold: str, - summary: str | None, -) -> str: +def render_review_body(*, pr_number: int, surfaced_count: int) -> str: """Compose the top-level review body. - Includes the agent's summary (if any) and a footer line so reviewers know - when findings were filtered out below the surfacing threshold. + Two fixed shapes — no agent prose: + + - 0 surfaced findings: a single "no issues" line. + - N surfaced findings: a single "found N potential issue(s)" line. """ - parts: list[str] = [] - 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.)" + if surfaced_count == 0: + headline = ( + "## ✅ Open SWE Review: No issues found\n\n" + "Open SWE reviewed this PR and found no potential bugs to report." ) 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"_{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) + issue_word = "issue" if surfaced_count == 1 else "issues" + headline = f"**Open SWE Review** found {surfaced_count} potential {issue_word}." + return f"{headline}\n\n" async def post_pull_request_review( diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 783dbd82..b7c397e2 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -29,7 +29,6 @@ from ..utils.github_token import get_github_token def publish_review( - summary: str | None = None, severity_threshold: str = "medium", cap: int = 4, ) -> dict[str, Any]: @@ -44,7 +43,8 @@ def publish_review( at ``cap`` to avoid review spam. 3. POST a single GitHub PR Review with the eligible findings as inline comments. ``finding.suggestion`` becomes a ```suggestion``` block - (the "Commit suggestion" UX). + (the "Commit suggestion" UX). The review body is a fixed, + host-formatted summary line — you do not write it. 4. Store the returned per-comment IDs back on each finding so a future re-review can resolve those threads on GitHub when the issues are fixed. 5. For findings whose status moved ``open`` → ``resolved`` since the last @@ -53,9 +53,6 @@ def publish_review( 6. Update ``last_reviewed_sha`` on the thread to the current head SHA. Args: - summary: Optional 1–2 sentence top-level take on the PR. Rendered as - the review body. Skip if you have nothing useful to say beyond - the per-finding comments. severity_threshold: Lowest severity to surface to GitHub (default ``medium``). Lower-severity findings stay in state and surface in the future UI but not on the PR. @@ -96,7 +93,6 @@ def publish_review( pr_number=pr_number, head_sha=head_sha, token=token, - summary=summary, severity_threshold=_cast_severity(severity_threshold), cap=cap, ) @@ -114,7 +110,6 @@ async def _publish_review_async( pr_number: int, head_sha: str, token: str, - summary: str | None, severity_threshold: Severity, cap: int, ) -> dict[str, Any]: @@ -145,9 +140,6 @@ async def _publish_review_async( review_body = render_review_body( pr_number=pr_number, surfaced_count=len(inline_comments), - total_open_count=len(open_unpublished), - severity_threshold=severity_threshold, - summary=summary, ) review_response = await post_pull_request_review( diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index beeaf911..2e327bda 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -67,56 +67,24 @@ def test_render_inline_comment_payload_returns_none_for_file_level() -> None: assert payload is None -def test_render_review_body_includes_summary_and_marker() -> None: - body = render_review_body( - pr_number=123, - surfaced_count=2, - total_open_count=3, - severity_threshold="medium", - summary="LGTM with two notes", - ) - assert "LGTM with two notes" in body +def test_render_review_body_with_findings_uses_potential_issue_phrasing() -> None: + body = render_review_body(pr_number=123, surfaced_count=2) + assert body.startswith("**Open SWE Review** found 2 potential issues.") assert "" 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: - body = render_review_body( - pr_number=99, - surfaced_count=0, - total_open_count=0, - severity_threshold="medium", - summary=None, - ) - assert "No issues found" in body +def test_render_review_body_singular_finding() -> None: + body = render_review_body(pr_number=123, surfaced_count=1) + assert body.startswith("**Open SWE Review** found 1 potential issue.") + + +def test_render_review_body_no_findings_message() -> None: + body = render_review_body(pr_number=99, surfaced_count=0) + assert "## ✅ Open SWE Review: No issues found" in body + assert "Open SWE reviewed this PR and found no potential bugs to report." 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 async def test_resolve_review_thread_returns_true_on_success() -> None: response = MagicMock() @@ -182,7 +150,6 @@ async def test_publish_review_skips_findings_already_published() -> None: pr_number=7, head_sha="sha", token="t", - summary=None, severity_threshold="medium", cap=15, ) @@ -222,7 +189,6 @@ async def test_publish_review_posts_summary_when_no_findings() -> None: pr_number=7, head_sha="sha", token="t", - summary=None, severity_threshold="medium", cap=15, )