mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-06 06:32:12 +00:00
fix(open-swe): always post a review summary, even with no findings (#1256)
* fix(reviewer): log every push/close early-return so 'silent ignore' is debuggable Pushes to PRs that haven't had a first review fall through the watch handler because the reviewer thread doesn't have kind=reviewer set. Without log lines on the early-return paths, this scenario was indistinguishable from 'webhook reached the handler at all' in the hosted log stream. Now every early-return logs at info or debug: - info when a real PR exists but the reviewer thread isn't set up (with a hint pointing at the trigger paths the user can use) - info when the repo isn't in the reviewer allowlist - debug for benign skips (non-branch refs, branch deletions, already-reviewed head_sha) * fix(reviewer): always post a summary review, even with no findings The publish_review tool gated POSTing on `inline_comments or summary`, so when the agent called publish_review() with no args on a clean PR the result returned `success: true` but no GitHub review was posted — the user got silence instead of a "no issues found" comment. - Drop the gate so publish_review always POSTs. - Friendlier no-findings render: `**No issues found.**` when the findings list is empty, vs. `**No issues at or above \`<sev>\` severity.**` with hidden count when only sub-threshold findings exist. Agent summary renders below. - Prompt now requires the agent to always pass a `summary` so the body is meaningful; calls out specifically not to skip on a clean PR.
This commit is contained in:
parent
378b95266e
commit
03d9f23645
5 changed files with 132 additions and 24 deletions
|
|
@ -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
|
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 PR Review with inline comments + suggestion blocks, and stores the
|
||||||
GitHub comment IDs back so re-reviews can later resolve threads.
|
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
|
### Re-reviewing on a new commit
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -93,18 +93,25 @@ def render_review_body(
|
||||||
when findings were filtered out below the surfacing threshold.
|
when findings were filtered out below the surfacing threshold.
|
||||||
"""
|
"""
|
||||||
parts: list[str] = []
|
parts: list[str] = []
|
||||||
if summary:
|
if surfaced_count == 0 and total_open_count == 0:
|
||||||
parts.append(summary.strip())
|
parts.append("**No issues found.**")
|
||||||
if surfaced_count == 0:
|
elif surfaced_count == 0:
|
||||||
parts.append("_No issues at or above the configured severity threshold._")
|
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:
|
else:
|
||||||
|
finding_word = "finding" if surfaced_count == 1 else "findings"
|
||||||
|
parts.append(f"**Found {surfaced_count} {finding_word}.**")
|
||||||
hidden = total_open_count - surfaced_count
|
hidden = total_open_count - surfaced_count
|
||||||
if hidden > 0:
|
if hidden > 0:
|
||||||
parts.append(
|
parts.append(
|
||||||
f"_Showing {surfaced_count} finding{'s' if surfaced_count != 1 else ''} "
|
f"_{hidden} lower-severity finding{'s' if hidden != 1 else ''} "
|
||||||
f"at severity ≥ `{severity_threshold}`; {hidden} lower-severity "
|
f"below `{severity_threshold}` hidden._"
|
||||||
f"finding{'s' if hidden != 1 else ''} hidden._"
|
|
||||||
)
|
)
|
||||||
|
if summary:
|
||||||
|
parts.append(summary.strip())
|
||||||
parts.append(f"<!-- open-swe-reviewer pr={pr_number} -->")
|
parts.append(f"<!-- open-swe-reviewer pr={pr_number} -->")
|
||||||
return "\n\n".join(p for p in parts if p)
|
return "\n\n".join(p for p in parts if p)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -150,20 +150,18 @@ async def _publish_review_async(
|
||||||
summary=summary,
|
summary=summary,
|
||||||
)
|
)
|
||||||
|
|
||||||
review_id: int | None = None
|
review_response = await post_pull_request_review(
|
||||||
if inline_comments or summary:
|
owner=owner,
|
||||||
review_response = await post_pull_request_review(
|
repo=repo,
|
||||||
owner=owner,
|
pr_number=pr_number,
|
||||||
repo=repo,
|
head_sha=head_sha,
|
||||||
pr_number=pr_number,
|
body=review_body,
|
||||||
head_sha=head_sha,
|
inline_comments=inline_comments,
|
||||||
body=review_body,
|
token=token,
|
||||||
inline_comments=inline_comments,
|
)
|
||||||
token=token,
|
if review_response is None:
|
||||||
)
|
return {"success": False, "error": "Failed to POST PR review"}
|
||||||
if review_response is None:
|
review_id = review_response.get("id") if isinstance(review_response, dict) else 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:
|
if review_id is not None and inline_comments:
|
||||||
comment_records = await fetch_review_comments(
|
comment_records = await fetch_review_comments(
|
||||||
|
|
|
||||||
|
|
@ -1640,6 +1640,12 @@ async def process_github_pr_close(payload: dict[str, Any]) -> None:
|
||||||
metadata = await _get_thread_metadata_safe(thread_id)
|
metadata = await _get_thread_metadata_safe(thread_id)
|
||||||
if metadata is None or metadata.get("kind") != REVIEWER_THREAD_KIND:
|
if metadata is None or metadata.get("kind") != REVIEWER_THREAD_KIND:
|
||||||
# No reviewer thread for this PR, nothing to do.
|
# 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
|
return
|
||||||
action = payload.get("action", "")
|
action = payload.get("action", "")
|
||||||
desired_watch = action == "reopened"
|
desired_watch = action == "reopened"
|
||||||
|
|
@ -1654,9 +1660,10 @@ async def process_github_push_event(payload: dict[str, Any]) -> None:
|
||||||
ref = payload.get("ref", "")
|
ref = payload.get("ref", "")
|
||||||
after_sha = payload.get("after", "")
|
after_sha = payload.get("after", "")
|
||||||
if not ref.startswith("refs/heads/"):
|
if not ref.startswith("refs/heads/"):
|
||||||
|
logger.debug("Push ignored: ref %s is not a branch", ref)
|
||||||
return
|
return
|
||||||
if not isinstance(after_sha, str) or not after_sha or set(after_sha) == {"0"}:
|
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
|
return
|
||||||
head_ref = ref[len("refs/heads/") :]
|
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", ""),
|
"name": repo.get("name", ""),
|
||||||
}
|
}
|
||||||
if not repo_config["owner"] or not repo_config["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
|
return
|
||||||
if not _is_repo_allowed_for_reviewer(repo_config):
|
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
|
return
|
||||||
|
|
||||||
app_token = await get_github_app_installation_token()
|
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)
|
head_sha = pr.get("head", {}).get("sha", after_sha)
|
||||||
pr_title = pr.get("title", "")
|
pr_title = pr.get("title", "")
|
||||||
if not isinstance(pr_number, int) or not base_sha or not head_sha:
|
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
|
return
|
||||||
|
|
||||||
thread_id = generate_reviewer_thread_id(repo_config["owner"], repo_config["name"], pr_number)
|
thread_id = generate_reviewer_thread_id(repo_config["owner"], repo_config["name"], pr_number)
|
||||||
metadata = await _get_thread_metadata_safe(thread_id)
|
metadata = await _get_thread_metadata_safe(thread_id)
|
||||||
if metadata is None or metadata.get("kind") != REVIEWER_THREAD_KIND:
|
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 <url>` or request "
|
||||||
|
"open-swe[bot] as a GitHub reviewer) to start watching.",
|
||||||
|
repo_config["owner"],
|
||||||
|
repo_config["name"],
|
||||||
|
pr_number,
|
||||||
|
)
|
||||||
return
|
return
|
||||||
if not metadata.get("watch"):
|
if not metadata.get("watch"):
|
||||||
logger.info("Push to %s ignored: reviewer thread %s is not watching", head_ref, thread_id)
|
logger.info("Push to %s ignored: reviewer thread %s is not watching", head_ref, thread_id)
|
||||||
|
|
|
||||||
|
|
@ -77,7 +77,8 @@ def test_render_review_body_includes_summary_and_marker() -> None:
|
||||||
)
|
)
|
||||||
assert "LGTM with two notes" in body
|
assert "LGTM with two notes" in body
|
||||||
assert "<!-- open-swe-reviewer pr=123 -->" in body
|
assert "<!-- open-swe-reviewer pr=123 -->" 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:
|
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",
|
severity_threshold="medium",
|
||||||
summary=None,
|
summary=None,
|
||||||
)
|
)
|
||||||
assert "No issues at or above" in body
|
assert "No issues found" 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
|
@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"]
|
posted = post_review.await_args.kwargs["inline_comments"]
|
||||||
paths = {c["path"] for c in posted}
|
paths = {c["path"] for c in posted}
|
||||||
assert paths == {"b.py"}
|
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
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue