mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-03 12:43:30 +00:00
fix: keep review check on follow-up commits, drop neutral conclusion (#1486)
* fix: keep review check visible on follow-up commits and stop using neutral The "Open SWE Review" check completed as `neutral` whenever findings were surfaced, which GitHub renders as a confusing "neutral check" group. Always complete it as `success` (informational/non-blocking), matching Devin and Corridor — the finding count stays in the title and findings post as comments. Also create a fresh check run on the new head SHA in the push re-review path: GitHub only shows check runs on a PR's current head, so the check vanished after a follow-up push (and the stale id settled on an outdated commit). Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: surface settled review check when push leaves diff unchanged --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
parent
1135e9342e
commit
259ec57186
4 changed files with 77 additions and 6 deletions
|
|
@ -119,13 +119,15 @@ async def complete_review_check_run(
|
||||||
def review_check_conclusion(surfaced_count: int) -> tuple[CheckConclusion, str, str]:
|
def review_check_conclusion(surfaced_count: int) -> tuple[CheckConclusion, str, str]:
|
||||||
"""Map a publish result to (conclusion, title, summary).
|
"""Map a publish result to (conclusion, title, summary).
|
||||||
|
|
||||||
Findings surfaced → ``neutral`` so the check never blocks merges; a clean
|
Always ``success`` so the check is informational and non-blocking, and so
|
||||||
review → ``success``.
|
GitHub groups it under "successful checks" rather than a confusing
|
||||||
|
"neutral check". The finding count is surfaced in the title; the findings
|
||||||
|
themselves are posted as PR comments.
|
||||||
"""
|
"""
|
||||||
if surfaced_count > 0:
|
if surfaced_count > 0:
|
||||||
issue_word = "issue" if surfaced_count == 1 else "issues"
|
issue_word = "issue" if surfaced_count == 1 else "issues"
|
||||||
return (
|
return (
|
||||||
"neutral",
|
"success",
|
||||||
f"Found {surfaced_count} potential {issue_word}",
|
f"Found {surfaced_count} potential {issue_word}",
|
||||||
f"Open SWE surfaced {surfaced_count} potential {issue_word} on this pull request.",
|
f"Open SWE surfaced {surfaced_count} potential {issue_word} on this pull request.",
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -60,7 +60,7 @@ from .utils.github_app import (
|
||||||
get_github_app_installation_token,
|
get_github_app_installation_token,
|
||||||
get_github_app_installation_token_with_expiry,
|
get_github_app_installation_token_with_expiry,
|
||||||
)
|
)
|
||||||
from .utils.github_checks import create_review_check_run
|
from .utils.github_checks import complete_review_check_run, create_review_check_run
|
||||||
from .utils.github_comments import (
|
from .utils.github_comments import (
|
||||||
OPEN_SWE_TAGS,
|
OPEN_SWE_TAGS,
|
||||||
GitHubAuthError,
|
GitHubAuthError,
|
||||||
|
|
@ -2385,6 +2385,29 @@ async def process_github_push_event(payload: dict[str, Any]) -> None:
|
||||||
)
|
)
|
||||||
):
|
):
|
||||||
await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha)
|
await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha)
|
||||||
|
# The old head's check disappears once the head moves (GitHub only
|
||||||
|
# shows checks on the current head), so even though no re-review runs,
|
||||||
|
# surface a settled check on the new head.
|
||||||
|
unchanged_check_id = await create_review_check_run(
|
||||||
|
owner=repo_config["owner"],
|
||||||
|
repo=repo_config["name"],
|
||||||
|
head_sha=head_sha,
|
||||||
|
token=app_token,
|
||||||
|
details_url=dashboard_thread_url(thread_id),
|
||||||
|
)
|
||||||
|
if unchanged_check_id is not None:
|
||||||
|
await complete_review_check_run(
|
||||||
|
owner=repo_config["owner"],
|
||||||
|
repo=repo_config["name"],
|
||||||
|
check_run_id=unchanged_check_id,
|
||||||
|
token=app_token,
|
||||||
|
conclusion="success",
|
||||||
|
title="No new changes to review",
|
||||||
|
summary=(
|
||||||
|
"The pull request diff is unchanged since the last reviewed "
|
||||||
|
f"commit {last_reviewed_sha}."
|
||||||
|
),
|
||||||
|
)
|
||||||
logger.info(
|
logger.info(
|
||||||
"Push to %s ignored: PR diff unchanged since last reviewed SHA %s",
|
"Push to %s ignored: PR diff unchanged since last reviewed SHA %s",
|
||||||
head_ref,
|
head_ref,
|
||||||
|
|
@ -2417,6 +2440,20 @@ async def process_github_push_event(payload: dict[str, Any]) -> None:
|
||||||
}
|
}
|
||||||
await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha)
|
await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha)
|
||||||
|
|
||||||
|
# GitHub only shows check runs on a PR's current head commit, so the check
|
||||||
|
# created on the previous head disappears after a follow-up push. Create a
|
||||||
|
# fresh in-progress check on the new head SHA so the review stays visible;
|
||||||
|
# publish (or the after-agent hook) settles this id.
|
||||||
|
check_run_id = await create_review_check_run(
|
||||||
|
owner=repo_config["owner"],
|
||||||
|
repo=repo_config["name"],
|
||||||
|
head_sha=head_sha,
|
||||||
|
token=app_token,
|
||||||
|
details_url=dashboard_thread_url(thread_id),
|
||||||
|
)
|
||||||
|
if check_run_id is not None:
|
||||||
|
await set_reviewer_thread_metadata(thread_id, extra={"review_check_run_id": check_run_id})
|
||||||
|
|
||||||
re_review_prompt = (
|
re_review_prompt = (
|
||||||
f"A new commit has been pushed to PR #{pr_number}. The new HEAD is "
|
f"A new commit has been pushed to PR #{pr_number}. The new HEAD is "
|
||||||
f"{head_sha}. Reconcile existing findings against the new diff, add any "
|
f"{head_sha}. Reconcile existing findings against the new diff, add any "
|
||||||
|
|
|
||||||
|
|
@ -117,11 +117,11 @@ def test_review_check_conclusion_mapping() -> None:
|
||||||
assert title == "No issues found"
|
assert title == "No issues found"
|
||||||
|
|
||||||
conclusion, title, _ = github_checks.review_check_conclusion(1)
|
conclusion, title, _ = github_checks.review_check_conclusion(1)
|
||||||
assert conclusion == "neutral"
|
assert conclusion == "success"
|
||||||
assert "1 potential issue" in title
|
assert "1 potential issue" in title
|
||||||
|
|
||||||
conclusion, title, _ = github_checks.review_check_conclusion(3)
|
conclusion, title, _ = github_checks.review_check_conclusion(3)
|
||||||
assert conclusion == "neutral"
|
assert conclusion == "success"
|
||||||
assert "3 potential issues" in title
|
assert "3 potential issues" in title
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -134,6 +134,16 @@ async def test_push_event_skips_when_pr_diff_unchanged_since_last_review() -> No
|
||||||
side_effect=["same diff", "same diff"],
|
side_effect=["same diff", "same diff"],
|
||||||
),
|
),
|
||||||
patch("agent.webapp.set_reviewer_thread_metadata", new=set_metadata),
|
patch("agent.webapp.set_reviewer_thread_metadata", new=set_metadata),
|
||||||
|
patch(
|
||||||
|
"agent.webapp.create_review_check_run",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value=42,
|
||||||
|
) as create_check,
|
||||||
|
patch(
|
||||||
|
"agent.webapp.complete_review_check_run",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value=True,
|
||||||
|
) as complete_check,
|
||||||
patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False),
|
patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False),
|
||||||
patch("agent.webapp.get_client", return_value=fake_client),
|
patch("agent.webapp.get_client", return_value=fake_client),
|
||||||
):
|
):
|
||||||
|
|
@ -142,6 +152,13 @@ async def test_push_event_skips_when_pr_diff_unchanged_since_last_review() -> No
|
||||||
fake_client.runs.create.assert_not_called()
|
fake_client.runs.create.assert_not_called()
|
||||||
set_metadata.assert_awaited_once()
|
set_metadata.assert_awaited_once()
|
||||||
assert set_metadata.await_args.kwargs["last_reviewed_sha"] == "newsha"
|
assert set_metadata.await_args.kwargs["last_reviewed_sha"] == "newsha"
|
||||||
|
# Even without a re-review, a settled check lands on the new head so the
|
||||||
|
# review stays visible after the head moves.
|
||||||
|
create_check.assert_awaited_once()
|
||||||
|
assert create_check.await_args.kwargs["head_sha"] == "newsha"
|
||||||
|
complete_check.assert_awaited_once()
|
||||||
|
assert complete_check.await_args.kwargs["check_run_id"] == 42
|
||||||
|
assert complete_check.await_args.kwargs["conclusion"] == "success"
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
|
|
@ -258,6 +275,11 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None:
|
||||||
"agent.webapp.set_reviewer_thread_metadata",
|
"agent.webapp.set_reviewer_thread_metadata",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
) as set_meta,
|
) as set_meta,
|
||||||
|
patch(
|
||||||
|
"agent.webapp.create_review_check_run",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value=99,
|
||||||
|
) as create_check,
|
||||||
patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False),
|
patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False),
|
||||||
patch("agent.webapp.get_client", return_value=fake_client),
|
patch("agent.webapp.get_client", return_value=fake_client),
|
||||||
):
|
):
|
||||||
|
|
@ -278,6 +300,16 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None:
|
||||||
if c.kwargs.get("head_sha") is not None
|
if c.kwargs.get("head_sha") is not None
|
||||||
]
|
]
|
||||||
assert "newsha" in head_sha_writes
|
assert "newsha" in head_sha_writes
|
||||||
|
# A fresh check run is created on the new head SHA (GitHub only shows
|
||||||
|
# checks on the current head), and its id is persisted for settling.
|
||||||
|
create_check.assert_awaited_once()
|
||||||
|
assert create_check.await_args.kwargs["head_sha"] == "newsha"
|
||||||
|
check_id_writes = [
|
||||||
|
c.kwargs.get("extra", {}).get("review_check_run_id")
|
||||||
|
for c in set_meta.await_args_list
|
||||||
|
if "review_check_run_id" in (c.kwargs.get("extra") or {})
|
||||||
|
]
|
||||||
|
assert 99 in check_id_writes
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue