mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-03 17:53:20 +00:00
feat: surface sub-threshold findings in review summary with web app link (#1571)
Instead of silently swallowing findings below the severity threshold, the review summary now mentions them with a count and links to the web app where they can be viewed. For example, if 2 low-severity findings are filtered out, the PR comment says "No issues found" and "2 additional findings can be viewed in the web app." with the existing [Open in Web] link. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
parent
2df0eabd35
commit
055b83e723
3 changed files with 107 additions and 4 deletions
|
|
@ -284,9 +284,16 @@ def render_review_body(
|
||||||
trace_url: str | None = None,
|
trace_url: str | None = None,
|
||||||
ui_url: str | None = None,
|
ui_url: str | None = None,
|
||||||
out_of_diff_findings: list[Finding] | None = None,
|
out_of_diff_findings: list[Finding] | None = None,
|
||||||
|
additional_findings_count: int = 0,
|
||||||
) -> str:
|
) -> str:
|
||||||
"""Compose the top-level review body."""
|
"""Compose the top-level review body.
|
||||||
|
|
||||||
|
When ``surfaced_count`` is 0 but ``additional_findings_count`` is > 0,
|
||||||
|
the headline says "No issues found" and a second line directs the reader
|
||||||
|
to the web app for the remaining sub-threshold findings.
|
||||||
|
"""
|
||||||
out_of_diff_findings = out_of_diff_findings or []
|
out_of_diff_findings = out_of_diff_findings or []
|
||||||
|
has_additional = additional_findings_count > 0
|
||||||
if surfaced_count == 0 and not out_of_diff_findings:
|
if surfaced_count == 0 and not out_of_diff_findings:
|
||||||
headline = (
|
headline = (
|
||||||
"## ✅ Open SWE Review: No issues found\n\n"
|
"## ✅ Open SWE Review: No issues found\n\n"
|
||||||
|
|
@ -299,6 +306,9 @@ def render_review_body(
|
||||||
headline = f"**Open SWE Review** found {surfaced_count} potential {issue_word}."
|
headline = f"**Open SWE Review** found {surfaced_count} potential {issue_word}."
|
||||||
|
|
||||||
parts = [headline]
|
parts = [headline]
|
||||||
|
if has_additional:
|
||||||
|
noun = "finding" if additional_findings_count == 1 else "findings"
|
||||||
|
parts.append(f"{additional_findings_count} additional {noun} can be viewed in the web app.")
|
||||||
if out_of_diff_findings:
|
if out_of_diff_findings:
|
||||||
parts.append(render_out_of_diff_section(out_of_diff_findings))
|
parts.append(render_out_of_diff_section(out_of_diff_findings))
|
||||||
links = []
|
links = []
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,7 @@ from langgraph.config import get_config
|
||||||
from ..dashboard.team_settings import get_team_review_trace_links_enabled
|
from ..dashboard.team_settings import get_team_review_trace_links_enabled
|
||||||
from ..reviewer_diff import compute_diff_line_set, fetch_pr_diff, is_range_in_diff
|
from ..reviewer_diff import compute_diff_line_set, fetch_pr_diff, is_range_in_diff
|
||||||
from ..reviewer_findings import (
|
from ..reviewer_findings import (
|
||||||
|
SEVERITY_ORDER,
|
||||||
Finding,
|
Finding,
|
||||||
ReviewerThreadMissingError,
|
ReviewerThreadMissingError,
|
||||||
Severity,
|
Severity,
|
||||||
|
|
@ -72,9 +73,10 @@ def publish_review(
|
||||||
GitHub Review but still resolves fixed threads and updates reviewer state.
|
GitHub Review but still resolves fixed threads and updates reviewer state.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
severity_threshold: Lowest severity to surface to GitHub (default
|
severity_threshold: Lowest severity to surface as inline GitHub comments
|
||||||
``medium``). Lower-severity findings stay in state and surface in
|
(default ``medium``). Lower-severity findings stay in state and are
|
||||||
the future UI but not on the PR.
|
mentioned in the review summary with a link to the web app, but are
|
||||||
|
not posted as inline PR comments.
|
||||||
cap: Maximum number of inline comments to publish (default 4).
|
cap: Maximum number of inline comments to publish (default 4).
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
|
|
@ -268,6 +270,16 @@ async def _publish_review_async(
|
||||||
in_diff_unpublished, severity_threshold=severity_threshold, cap=cap
|
in_diff_unpublished, severity_threshold=severity_threshold, cap=cap
|
||||||
)
|
)
|
||||||
|
|
||||||
|
severity_rank = SEVERITY_ORDER[severity_threshold]
|
||||||
|
eligible_ids = {f.get("id") for f in eligible}
|
||||||
|
additional_findings_count = sum(
|
||||||
|
1
|
||||||
|
for f in in_diff_unpublished
|
||||||
|
if f.get("id") not in eligible_ids
|
||||||
|
and f.get("status", "open") == "open"
|
||||||
|
and SEVERITY_ORDER.get(f.get("severity", "low"), 0) < severity_rank
|
||||||
|
)
|
||||||
|
|
||||||
inline_comments: list[dict[str, Any]] = []
|
inline_comments: list[dict[str, Any]] = []
|
||||||
eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]] = []
|
eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]] = []
|
||||||
for finding in eligible:
|
for finding in eligible:
|
||||||
|
|
@ -328,6 +340,7 @@ async def _publish_review_async(
|
||||||
surfaced_count=len(inline_comments),
|
surfaced_count=len(inline_comments),
|
||||||
trace_url=review_trace_url,
|
trace_url=review_trace_url,
|
||||||
ui_url=review_ui_url,
|
ui_url=review_ui_url,
|
||||||
|
additional_findings_count=additional_findings_count,
|
||||||
)
|
)
|
||||||
|
|
||||||
review_response = await post_pull_request_review(
|
review_response = await post_pull_request_review(
|
||||||
|
|
@ -362,6 +375,7 @@ async def _publish_review_async(
|
||||||
surfaced_count=len(retry_inline),
|
surfaced_count=len(retry_inline),
|
||||||
trace_url=review_trace_url,
|
trace_url=review_trace_url,
|
||||||
ui_url=review_ui_url,
|
ui_url=review_ui_url,
|
||||||
|
additional_findings_count=additional_findings_count,
|
||||||
)
|
)
|
||||||
retry_response = await post_pull_request_review(
|
retry_response = await post_pull_request_review(
|
||||||
owner=owner,
|
owner=owner,
|
||||||
|
|
|
||||||
|
|
@ -212,6 +212,40 @@ def test_render_review_body_no_findings_message() -> None:
|
||||||
body = render_review_body(pr_number=99, surfaced_count=0)
|
body = render_review_body(pr_number=99, surfaced_count=0)
|
||||||
assert "## ✅ Open SWE Review: No issues found" in body
|
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 reviewed this PR and found no potential bugs to report." in body
|
||||||
|
assert "additional" not in body
|
||||||
|
|
||||||
|
|
||||||
|
def test_render_review_body_with_additional_findings_and_ui_link() -> None:
|
||||||
|
body = render_review_body(
|
||||||
|
pr_number=99,
|
||||||
|
surfaced_count=0,
|
||||||
|
additional_findings_count=2,
|
||||||
|
ui_url="https://dash.example/agents/reviews/o/r/99",
|
||||||
|
)
|
||||||
|
assert "## ✅ Open SWE Review: No issues found" in body
|
||||||
|
assert "2 additional findings can be viewed in the web app." in body
|
||||||
|
assert "[Open in Web](https://dash.example/agents/reviews/o/r/99)" in body
|
||||||
|
|
||||||
|
|
||||||
|
def test_render_review_body_with_single_additional_finding_uses_singular() -> None:
|
||||||
|
body = render_review_body(pr_number=99, surfaced_count=0, additional_findings_count=1)
|
||||||
|
assert "1 additional finding can be viewed in the web app." in body
|
||||||
|
|
||||||
|
|
||||||
|
def test_render_review_body_with_surfaced_and_additional_findings() -> None:
|
||||||
|
body = render_review_body(
|
||||||
|
pr_number=99,
|
||||||
|
surfaced_count=3,
|
||||||
|
additional_findings_count=2,
|
||||||
|
ui_url="https://dash.example/agents/reviews/o/r/99",
|
||||||
|
)
|
||||||
|
assert "found 3 potential issues." in body
|
||||||
|
assert "2 additional findings can be viewed in the web app." in body
|
||||||
|
|
||||||
|
|
||||||
|
def test_render_review_body_additional_findings_zero_omits_line() -> None:
|
||||||
|
body = render_review_body(pr_number=99, surfaced_count=0, additional_findings_count=0)
|
||||||
|
assert "additional" not in body
|
||||||
|
|
||||||
|
|
||||||
def test_render_status_comment_reviewing_includes_ui_link(monkeypatch: Any) -> None:
|
def test_render_status_comment_reviewing_includes_ui_link(monkeypatch: Any) -> None:
|
||||||
|
|
@ -387,6 +421,51 @@ def test_publish_review_eval_mode_does_not_call_github() -> None:
|
||||||
set_meta.assert_awaited_once_with("tid", last_reviewed_sha="sha")
|
set_meta.assert_awaited_once_with("tid", last_reviewed_sha="sha")
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_publish_review_surfaces_additional_findings_count_in_body() -> None:
|
||||||
|
"""When all surfaced findings are above threshold but sub-threshold findings
|
||||||
|
exist, the review body must mention how many additional findings are in the
|
||||||
|
web app."""
|
||||||
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
|
findings = [
|
||||||
|
_f(id="f_low_1", severity="low", file="a.py", start_line=1, end_line=1),
|
||||||
|
_f(id="f_low_2", severity="low", file="b.py", start_line=2, end_line=2),
|
||||||
|
]
|
||||||
|
post_review = AsyncMock(return_value={"id": 555})
|
||||||
|
fetch_comments = AsyncMock(return_value=[])
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
|
||||||
|
patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)),
|
||||||
|
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", AsyncMock()),
|
||||||
|
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
|
||||||
|
):
|
||||||
|
result = await _publish_review_async(
|
||||||
|
owner="o",
|
||||||
|
repo="r",
|
||||||
|
pr_number=7,
|
||||||
|
head_sha="sha",
|
||||||
|
token="t",
|
||||||
|
severity_threshold="medium",
|
||||||
|
cap=15,
|
||||||
|
is_re_review=False,
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result["success"] is True
|
||||||
|
assert result["surfaced_count"] == 0
|
||||||
|
posted_body = post_review.await_args.kwargs["body"]
|
||||||
|
assert "No issues found" in posted_body
|
||||||
|
assert "2 additional findings can be viewed in the web app." in posted_body
|
||||||
|
|
||||||
|
|
||||||
def test_publish_review_forwards_trace_link_config_override() -> None:
|
def test_publish_review_forwards_trace_link_config_override() -> None:
|
||||||
from agent.tools.publish_review import publish_review
|
from agent.tools.publish_review import publish_review
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue