mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 19:43:15 +00:00
fix(open-swe): host-format the review summary, drop agent prose (#1257)
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.
This commit is contained in:
parent
03d9f23645
commit
88a55057ea
4 changed files with 32 additions and 93 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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"<!-- open-swe-reviewer pr={pr_number} -->")
|
||||
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<!-- open-swe-reviewer pr={pr_number} -->"
|
||||
|
||||
|
||||
async def post_pull_request_review(
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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 "<!-- open-swe-reviewer pr=123 -->" 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 "<!-- open-swe-reviewer pr=99 -->" 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,
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue