mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-04 19:32:12 +00:00
feat(reviewer): add verdict-aware review checks
This commit is contained in:
parent
9a2535a9ec
commit
39c768aa3f
7 changed files with 389 additions and 55 deletions
|
|
@ -28,7 +28,7 @@ async def settle_review_check_on_exit(
|
||||||
state: AgentState,
|
state: AgentState,
|
||||||
runtime: Runtime,
|
runtime: Runtime,
|
||||||
) -> dict[str, Any] | None:
|
) -> dict[str, Any] | None:
|
||||||
"""Fail the tracked review check run if the run ended without publishing."""
|
"""Neutralize the tracked review check if the run ended without publishing."""
|
||||||
config = get_config()
|
config = get_config()
|
||||||
configurable = config.get("configurable", {})
|
configurable = config.get("configurable", {})
|
||||||
if not isinstance(configurable, dict):
|
if not isinstance(configurable, dict):
|
||||||
|
|
|
||||||
|
|
@ -403,7 +403,23 @@ async def _publish_review_async(
|
||||||
)
|
)
|
||||||
await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha)
|
await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha)
|
||||||
await clear_review_started_comment(thread_id=thread_id, owner=owner, repo=repo, token=token)
|
await clear_review_started_comment(thread_id=thread_id, owner=owner, repo=repo, token=token)
|
||||||
conclusion, check_title, check_summary = review_check_conclusion(0)
|
skip_result: dict[str, Any] = {
|
||||||
|
"success": True,
|
||||||
|
"review_id": None,
|
||||||
|
"surfaced_count": 0,
|
||||||
|
"hidden_count": max(len(open_unpublished), 0),
|
||||||
|
"resolved_thread_count": resolved_thread_count,
|
||||||
|
"skipped_empty_re_review": True,
|
||||||
|
"blocking_finding_count": sum(
|
||||||
|
1 for finding in findings if _finding_blocks_verdict(finding)
|
||||||
|
),
|
||||||
|
"verdict_authorization": verdict_authorization,
|
||||||
|
}
|
||||||
|
if verdict_ignored_reason:
|
||||||
|
skip_result["verdict_submitted"] = False
|
||||||
|
skip_result["verdict_ignored"] = True
|
||||||
|
skip_result["verdict_ignored_reason"] = verdict_ignored_reason
|
||||||
|
conclusion, check_title, check_summary = review_check_conclusion(skip_result)
|
||||||
await settle_review_check_run(
|
await settle_review_check_run(
|
||||||
thread_id=thread_id,
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
|
|
@ -413,18 +429,6 @@ async def _publish_review_async(
|
||||||
title=check_title,
|
title=check_title,
|
||||||
summary=check_summary,
|
summary=check_summary,
|
||||||
)
|
)
|
||||||
skip_result: dict[str, Any] = {
|
|
||||||
"success": True,
|
|
||||||
"review_id": None,
|
|
||||||
"surfaced_count": 0,
|
|
||||||
"hidden_count": max(len(open_unpublished), 0),
|
|
||||||
"resolved_thread_count": resolved_thread_count,
|
|
||||||
"skipped_empty_re_review": True,
|
|
||||||
}
|
|
||||||
if verdict_ignored_reason:
|
|
||||||
skip_result["verdict_submitted"] = False
|
|
||||||
skip_result["verdict_ignored"] = True
|
|
||||||
skip_result["verdict_ignored_reason"] = verdict_ignored_reason
|
|
||||||
return skip_result
|
return skip_result
|
||||||
|
|
||||||
review_body = render_review_body(
|
review_body = render_review_body(
|
||||||
|
|
@ -695,16 +699,6 @@ async def _publish_review_async(
|
||||||
|
|
||||||
await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha)
|
await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha)
|
||||||
await clear_review_started_comment(thread_id=thread_id, owner=owner, repo=repo, token=token)
|
await clear_review_started_comment(thread_id=thread_id, owner=owner, repo=repo, token=token)
|
||||||
conclusion, check_title, check_summary = review_check_conclusion(len(inline_comments))
|
|
||||||
await settle_review_check_run(
|
|
||||||
thread_id=thread_id,
|
|
||||||
owner=owner,
|
|
||||||
repo=repo,
|
|
||||||
token=token,
|
|
||||||
conclusion=conclusion,
|
|
||||||
title=check_title,
|
|
||||||
summary=check_summary,
|
|
||||||
)
|
|
||||||
|
|
||||||
result: dict[str, Any] = {
|
result: dict[str, Any] = {
|
||||||
"success": True,
|
"success": True,
|
||||||
|
|
@ -712,6 +706,10 @@ async def _publish_review_async(
|
||||||
"surfaced_count": len(inline_comments),
|
"surfaced_count": len(inline_comments),
|
||||||
"hidden_count": max(len(open_unpublished) - len(inline_comments), 0),
|
"hidden_count": max(len(open_unpublished) - len(inline_comments), 0),
|
||||||
"resolved_thread_count": resolved_thread_count,
|
"resolved_thread_count": resolved_thread_count,
|
||||||
|
"blocking_finding_count": sum(
|
||||||
|
1 for finding in findings if _finding_blocks_verdict(finding)
|
||||||
|
),
|
||||||
|
"verdict_authorization": verdict_authorization,
|
||||||
}
|
}
|
||||||
if recorded_state:
|
if recorded_state:
|
||||||
result["review_state"] = recorded_state
|
result["review_state"] = recorded_state
|
||||||
|
|
@ -734,6 +732,16 @@ async def _publish_review_async(
|
||||||
"Some findings had anchors not in the PR diff; "
|
"Some findings had anchors not in the PR diff; "
|
||||||
"call update_finding to fix or resolve them."
|
"call update_finding to fix or resolve them."
|
||||||
)
|
)
|
||||||
|
conclusion, check_title, check_summary = review_check_conclusion(result)
|
||||||
|
await settle_review_check_run(
|
||||||
|
thread_id=thread_id,
|
||||||
|
owner=owner,
|
||||||
|
repo=repo,
|
||||||
|
token=token,
|
||||||
|
conclusion=conclusion,
|
||||||
|
title=check_title,
|
||||||
|
summary=check_summary,
|
||||||
|
)
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -12,6 +12,7 @@ break review dispatch or publish.
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import logging
|
import logging
|
||||||
|
from collections.abc import Mapping
|
||||||
from datetime import UTC, datetime
|
from datetime import UTC, datetime
|
||||||
from typing import Literal
|
from typing import Literal
|
||||||
|
|
||||||
|
|
@ -153,23 +154,75 @@ async def post_autofix_status_check(
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
||||||
def review_check_conclusion(surfaced_count: int) -> tuple[CheckConclusion, str, str]:
|
def review_check_conclusion(
|
||||||
"""Map a publish result to (conclusion, title, summary).
|
publish_outcome: Mapping[str, object],
|
||||||
|
) -> tuple[CheckConclusion, str, str]:
|
||||||
Always ``success`` so the check is informational and non-blocking, and so
|
"""Map the authoritative publish outcome to a check conclusion."""
|
||||||
GitHub groups it under "successful checks" rather than a confusing
|
verdict_submitted = publish_outcome.get("verdict_submitted") is True
|
||||||
"neutral check". The finding count is surfaced in the title; the findings
|
verdict_event = publish_outcome.get("verdict_event")
|
||||||
themselves are posted as PR comments.
|
if verdict_submitted and verdict_event == "APPROVE":
|
||||||
"""
|
|
||||||
if surfaced_count > 0:
|
|
||||||
issue_word = "issue" if surfaced_count == 1 else "issues"
|
|
||||||
return (
|
return (
|
||||||
"success",
|
"success",
|
||||||
f"Found {surfaced_count} potential {issue_word}",
|
"Review approved",
|
||||||
f"Open SWE surfaced {surfaced_count} potential {issue_word} on this pull request.",
|
"Open SWE recorded an approving review on this pull request.",
|
||||||
)
|
)
|
||||||
return (
|
if verdict_submitted and verdict_event == "REQUEST_CHANGES":
|
||||||
"success",
|
return (
|
||||||
"No issues found",
|
"failure",
|
||||||
"Open SWE reviewed this pull request and found no issues.",
|
"Changes requested",
|
||||||
|
"Open SWE recorded a request-changes review on this pull request.",
|
||||||
|
)
|
||||||
|
|
||||||
|
blocking_count_raw = publish_outcome.get("blocking_finding_count")
|
||||||
|
blocking_count = (
|
||||||
|
blocking_count_raw
|
||||||
|
if isinstance(blocking_count_raw, int) and not isinstance(blocking_count_raw, bool)
|
||||||
|
else None
|
||||||
|
)
|
||||||
|
ignored_reason = publish_outcome.get("verdict_ignored_reason")
|
||||||
|
if ignored_reason and ignored_reason != "self_review":
|
||||||
|
return (
|
||||||
|
"neutral",
|
||||||
|
"Verdict withheld",
|
||||||
|
f"Open SWE published without a verdict ({str(ignored_reason).replace('_', ' ')}).",
|
||||||
|
)
|
||||||
|
if blocking_count == 0:
|
||||||
|
return (
|
||||||
|
"success",
|
||||||
|
"No issues found",
|
||||||
|
"Open SWE reviewed this pull request and found no blocking issues.",
|
||||||
|
)
|
||||||
|
|
||||||
|
verdict_authorization = publish_outcome.get("verdict_authorization")
|
||||||
|
if (
|
||||||
|
blocking_count is not None
|
||||||
|
and blocking_count > 0
|
||||||
|
and verdict_authorization
|
||||||
|
in {
|
||||||
|
"requested",
|
||||||
|
"consistent",
|
||||||
|
}
|
||||||
|
):
|
||||||
|
issue_word = "issue" if blocking_count == 1 else "issues"
|
||||||
|
return (
|
||||||
|
"failure",
|
||||||
|
f"Found {blocking_count} blocking {issue_word}",
|
||||||
|
f"Open SWE found {blocking_count} blocking {issue_word} on this pull request.",
|
||||||
|
)
|
||||||
|
|
||||||
|
surfaced_count_raw = publish_outcome.get("surfaced_count")
|
||||||
|
surfaced_count = (
|
||||||
|
surfaced_count_raw
|
||||||
|
if isinstance(surfaced_count_raw, int) and not isinstance(surfaced_count_raw, bool)
|
||||||
|
else 0
|
||||||
|
)
|
||||||
|
if surfaced_count > 0:
|
||||||
|
issue_word = "issue" if surfaced_count == 1 else "issues"
|
||||||
|
title = f"Found {surfaced_count} potential {issue_word}"
|
||||||
|
else:
|
||||||
|
title = "Review completed without verdict"
|
||||||
|
return (
|
||||||
|
"neutral",
|
||||||
|
title,
|
||||||
|
"Open SWE completed the review without an authoritative verdict.",
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -159,6 +159,7 @@ async def trigger_pr_review_from_ref(
|
||||||
langgraph_client = common.get_client(url=common.LANGGRAPH_URL)
|
langgraph_client = common.get_client(url=common.LANGGRAPH_URL)
|
||||||
if not await common._ensure_thread_exists_for_metadata(thread_id, langgraph_client):
|
if not await common._ensure_thread_exists_for_metadata(thread_id, langgraph_client):
|
||||||
return {"success": False, "error": "Could not create reviewer thread"}
|
return {"success": False, "error": "Could not create reviewer thread"}
|
||||||
|
existing_metadata = await common._get_thread_metadata_safe(thread_id) or {}
|
||||||
|
|
||||||
pr_meta: ReviewerPRMeta = {
|
pr_meta: ReviewerPRMeta = {
|
||||||
"owner": pr_ref.owner,
|
"owner": pr_ref.owner,
|
||||||
|
|
@ -179,6 +180,20 @@ async def trigger_pr_review_from_ref(
|
||||||
await common.set_reviewer_thread_metadata(
|
await common.set_reviewer_thread_metadata(
|
||||||
thread_id, pr=pr_meta, watch=True, slack_thread=slack_thread_meta, head_sha=head_sha
|
thread_id, pr=pr_meta, watch=True, slack_thread=slack_thread_meta, head_sha=head_sha
|
||||||
)
|
)
|
||||||
|
existing_check_id = existing_metadata.get("review_check_run_id")
|
||||||
|
existing_check_head = existing_metadata.get("head_sha")
|
||||||
|
if not (isinstance(existing_check_id, int) and existing_check_head == head_sha):
|
||||||
|
check_run_id = await common.create_review_check_run(
|
||||||
|
owner=pr_ref.owner,
|
||||||
|
repo=pr_ref.repo,
|
||||||
|
head_sha=head_sha,
|
||||||
|
token=app_token,
|
||||||
|
details_url=common.dashboard_thread_url(thread_id),
|
||||||
|
)
|
||||||
|
if check_run_id is not None:
|
||||||
|
await common.set_reviewer_thread_metadata(
|
||||||
|
thread_id, extra={"review_check_run_id": check_run_id}
|
||||||
|
)
|
||||||
await common.post_review_started_comment(
|
await common.post_review_started_comment(
|
||||||
thread_id=thread_id,
|
thread_id=thread_id,
|
||||||
owner=pr_ref.owner,
|
owner=pr_ref.owner,
|
||||||
|
|
|
||||||
|
|
@ -1,10 +1,12 @@
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
from unittest.mock import AsyncMock, patch
|
||||||
|
|
||||||
import httpx
|
import httpx
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
|
from agent.middleware.settle_review_check import settle_review_check_on_exit
|
||||||
from agent.review import publish as reviewer_publish
|
from agent.review import publish as reviewer_publish
|
||||||
from agent.utils import github_checks
|
from agent.utils import github_checks
|
||||||
|
|
||||||
|
|
@ -146,18 +148,86 @@ async def test_post_autofix_status_check_completes_neutral(
|
||||||
assert body["details_url"] == "https://example.com/thread"
|
assert body["details_url"] == "https://example.com/thread"
|
||||||
|
|
||||||
|
|
||||||
def test_review_check_conclusion_mapping() -> None:
|
@pytest.mark.parametrize(
|
||||||
conclusion, title, _ = github_checks.review_check_conclusion(0)
|
("outcome", "expected_conclusion", "expected_title"),
|
||||||
assert conclusion == "success"
|
[
|
||||||
assert title == "No issues found"
|
(
|
||||||
|
{
|
||||||
conclusion, title, _ = github_checks.review_check_conclusion(1)
|
"verdict_submitted": True,
|
||||||
assert conclusion == "success"
|
"verdict_event": "APPROVE",
|
||||||
assert "1 potential issue" in title
|
"blocking_finding_count": 2,
|
||||||
|
"verdict_authorization": "requested",
|
||||||
conclusion, title, _ = github_checks.review_check_conclusion(3)
|
},
|
||||||
assert conclusion == "success"
|
"success",
|
||||||
assert "3 potential issues" in title
|
"Review approved",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{
|
||||||
|
"verdict_submitted": True,
|
||||||
|
"verdict_event": "REQUEST_CHANGES",
|
||||||
|
"blocking_finding_count": 0,
|
||||||
|
"verdict_authorization": "requested",
|
||||||
|
},
|
||||||
|
"failure",
|
||||||
|
"Changes requested",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{"blocking_finding_count": 0, "verdict_authorization": "none"},
|
||||||
|
"success",
|
||||||
|
"No issues found",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{"blocking_finding_count": 2, "verdict_authorization": "consistent"},
|
||||||
|
"failure",
|
||||||
|
"Found 2 blocking issues",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{
|
||||||
|
"blocking_finding_count": 1,
|
||||||
|
"verdict_authorization": "requested",
|
||||||
|
"verdict_ignored_reason": "self_review",
|
||||||
|
},
|
||||||
|
"failure",
|
||||||
|
"Found 1 blocking issue",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{
|
||||||
|
"blocking_finding_count": 0,
|
||||||
|
"verdict_authorization": "consistent",
|
||||||
|
"verdict_ignored_reason": "self_review",
|
||||||
|
},
|
||||||
|
"success",
|
||||||
|
"No issues found",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{
|
||||||
|
"blocking_finding_count": 1,
|
||||||
|
"verdict_authorization": "consistent",
|
||||||
|
"verdict_ignored_reason": "head_moved",
|
||||||
|
},
|
||||||
|
"neutral",
|
||||||
|
"Verdict withheld",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
{
|
||||||
|
"blocking_finding_count": 3,
|
||||||
|
"surfaced_count": 3,
|
||||||
|
"verdict_authorization": "none",
|
||||||
|
},
|
||||||
|
"neutral",
|
||||||
|
"Found 3 potential issues",
|
||||||
|
),
|
||||||
|
({}, "neutral", "Review completed without verdict"),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_review_check_conclusion_mapping(
|
||||||
|
outcome: dict[str, object],
|
||||||
|
expected_conclusion: str,
|
||||||
|
expected_title: str,
|
||||||
|
) -> None:
|
||||||
|
conclusion, title, _ = github_checks.review_check_conclusion(outcome)
|
||||||
|
assert conclusion == expected_conclusion
|
||||||
|
assert title == expected_title
|
||||||
|
|
||||||
|
|
||||||
async def test_settle_review_check_run_noop_without_tracked_id(
|
async def test_settle_review_check_run_noop_without_tracked_id(
|
||||||
|
|
@ -269,3 +339,66 @@ async def test_settle_review_check_run_keeps_id_on_patch_failure(
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
async def test_settle_review_check_on_exit_without_publish_is_neutral() -> None:
|
||||||
|
settle = AsyncMock()
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"agent.middleware.settle_review_check.get_config",
|
||||||
|
return_value={
|
||||||
|
"configurable": {
|
||||||
|
"thread_id": "t1",
|
||||||
|
"repo": {"owner": "acme", "name": "widgets"},
|
||||||
|
}
|
||||||
|
},
|
||||||
|
),
|
||||||
|
patch(
|
||||||
|
"agent.middleware.settle_review_check.get_thread_metadata",
|
||||||
|
AsyncMock(return_value={"review_check_run_id": 42}),
|
||||||
|
),
|
||||||
|
patch("agent.middleware.settle_review_check.get_github_token", return_value="tok"),
|
||||||
|
patch("agent.middleware.settle_review_check.settle_review_check_run", settle),
|
||||||
|
):
|
||||||
|
await settle_review_check_on_exit.aafter_agent({}, None)
|
||||||
|
|
||||||
|
settle.assert_awaited_once()
|
||||||
|
assert settle.await_args.kwargs["conclusion"] == "neutral"
|
||||||
|
assert settle.await_args.kwargs["title"] == "Review did not complete"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("conclusion", ["success", "neutral", "failure"])
|
||||||
|
async def test_settle_review_check_on_exit_preserves_pending_conclusion(
|
||||||
|
conclusion: str,
|
||||||
|
) -> None:
|
||||||
|
settle = AsyncMock()
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"agent.middleware.settle_review_check.get_config",
|
||||||
|
return_value={
|
||||||
|
"configurable": {
|
||||||
|
"thread_id": "t1",
|
||||||
|
"repo": {"owner": "acme", "name": "widgets"},
|
||||||
|
}
|
||||||
|
},
|
||||||
|
),
|
||||||
|
patch(
|
||||||
|
"agent.middleware.settle_review_check.get_thread_metadata",
|
||||||
|
AsyncMock(
|
||||||
|
return_value={
|
||||||
|
"review_check_run_id": 42,
|
||||||
|
"review_check_pending_result": {
|
||||||
|
"conclusion": conclusion,
|
||||||
|
"title": "Published result",
|
||||||
|
"summary": "Authoritative outcome",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
),
|
||||||
|
),
|
||||||
|
patch("agent.middleware.settle_review_check.get_github_token", return_value="tok"),
|
||||||
|
patch("agent.middleware.settle_review_check.settle_review_check_run", settle),
|
||||||
|
):
|
||||||
|
await settle_review_check_on_exit.aafter_agent({}, None)
|
||||||
|
|
||||||
|
assert settle.await_args.kwargs["conclusion"] == conclusion
|
||||||
|
assert settle.await_args.kwargs["title"] == "Published result"
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,7 @@ import hmac
|
||||||
import importlib
|
import importlib
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
|
from unittest.mock import AsyncMock
|
||||||
|
|
||||||
from fastapi.testclient import TestClient
|
from fastapi.testclient import TestClient
|
||||||
|
|
||||||
|
|
@ -1006,6 +1007,7 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None:
|
||||||
|
|
||||||
def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
||||||
captured: dict[str, object] = {}
|
captured: dict[str, object] = {}
|
||||||
|
metadata_writes: list[dict[str, object]] = []
|
||||||
auto_review_checked = False
|
auto_review_checked = False
|
||||||
|
|
||||||
async def fake_auto_review_enabled(_repo_config: dict[str, str]) -> bool:
|
async def fake_auto_review_enabled(_repo_config: dict[str, str]) -> bool:
|
||||||
|
|
@ -1058,7 +1060,14 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
||||||
|
|
||||||
async def fake_set_reviewer_thread_metadata(thread_id: str, **kwargs: object) -> None:
|
async def fake_set_reviewer_thread_metadata(thread_id: str, **kwargs: object) -> None:
|
||||||
captured["set_metadata_thread_id"] = thread_id
|
captured["set_metadata_thread_id"] = thread_id
|
||||||
captured["set_metadata_kwargs"] = kwargs
|
metadata_writes.append(kwargs)
|
||||||
|
|
||||||
|
async def fake_get_thread_metadata_safe(_thread_id: str) -> dict[str, object]:
|
||||||
|
return {}
|
||||||
|
|
||||||
|
async def fake_create_review_check_run(**kwargs: object) -> int:
|
||||||
|
captured["check_run_kwargs"] = kwargs
|
||||||
|
return 77
|
||||||
|
|
||||||
monkeypatch.setattr(webhook_common, "_is_repo_auto_review_enabled", fake_auto_review_enabled)
|
monkeypatch.setattr(webhook_common, "_is_repo_auto_review_enabled", fake_auto_review_enabled)
|
||||||
monkeypatch.setattr(
|
monkeypatch.setattr(
|
||||||
|
|
@ -1079,9 +1088,11 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
||||||
|
|
||||||
monkeypatch.setattr(webhook_common, "fetch_github_pr_metadata", fake_fetch_github_pr_metadata)
|
monkeypatch.setattr(webhook_common, "fetch_github_pr_metadata", fake_fetch_github_pr_metadata)
|
||||||
monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", fake_cache_github_token)
|
monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", fake_cache_github_token)
|
||||||
|
monkeypatch.setattr(webhook_common, "_get_thread_metadata_safe", fake_get_thread_metadata_safe)
|
||||||
monkeypatch.setattr(
|
monkeypatch.setattr(
|
||||||
webhook_common, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata
|
webhook_common, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata
|
||||||
)
|
)
|
||||||
|
monkeypatch.setattr(webhook_common, "create_review_check_run", fake_create_review_check_run)
|
||||||
monkeypatch.setattr(
|
monkeypatch.setattr(
|
||||||
webhook_common, "post_review_started_comment", fake_post_review_started_comment
|
webhook_common, "post_review_started_comment", fake_post_review_started_comment
|
||||||
)
|
)
|
||||||
|
|
@ -1124,7 +1135,15 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
||||||
}
|
}
|
||||||
# The live head must be persisted to metadata so resolve_review_head_sha
|
# The live head must be persisted to metadata so resolve_review_head_sha
|
||||||
# doesn't return a stale head left by a prior push/ready dispatch.
|
# doesn't return a stale head left by a prior push/ready dispatch.
|
||||||
assert captured["set_metadata_kwargs"]["head_sha"] == "head-sha"
|
assert any(write.get("head_sha") == "head-sha" for write in metadata_writes)
|
||||||
|
assert captured["check_run_kwargs"] == {
|
||||||
|
"owner": "langchain-ai",
|
||||||
|
"repo": "open-swe",
|
||||||
|
"head_sha": "head-sha",
|
||||||
|
"token": "app-token",
|
||||||
|
"details_url": webhook_common.dashboard_thread_url(str(captured["thread_id"])),
|
||||||
|
}
|
||||||
|
assert any(write.get("extra") == {"review_check_run_id": 77} for write in metadata_writes)
|
||||||
# A live status comment is posted on dispatch so the PR shows "reviewing".
|
# A live status comment is posted on dispatch so the PR shows "reviewing".
|
||||||
assert captured["status_comment_kwargs"]["pr_number"] == 1244
|
assert captured["status_comment_kwargs"]["pr_number"] == 1244
|
||||||
assert config["verdict_authorized"] is True
|
assert config["verdict_authorized"] is True
|
||||||
|
|
@ -1136,6 +1155,65 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def test_trigger_pr_review_from_ref_reuses_open_check_on_same_head(monkeypatch) -> None:
|
||||||
|
created_check = AsyncMock(return_value=88)
|
||||||
|
|
||||||
|
async def fake_token() -> tuple[str | None, str | None]:
|
||||||
|
return "app-token", None
|
||||||
|
|
||||||
|
async def fake_metadata(pr_ref: GitHubPrRef, *, token: str) -> dict[str, object]:
|
||||||
|
return {
|
||||||
|
"html_url": pr_ref.url,
|
||||||
|
"base": {"sha": "base-sha"},
|
||||||
|
"head": {"sha": "head-sha", "ref": "feature-branch"},
|
||||||
|
}
|
||||||
|
|
||||||
|
class _FakeRunsClient:
|
||||||
|
async def create(self, *_args: object, **_kwargs: object) -> dict[str, str]:
|
||||||
|
return {"run_id": "run-1"}
|
||||||
|
|
||||||
|
class _FakeThreadsClient:
|
||||||
|
async def create(self, **_kwargs: object) -> None:
|
||||||
|
return None
|
||||||
|
|
||||||
|
class _FakeLangGraphClient:
|
||||||
|
runs = _FakeRunsClient()
|
||||||
|
threads = _FakeThreadsClient()
|
||||||
|
|
||||||
|
monkeypatch.setattr(webhook_common, "get_github_app_installation_token_with_expiry", fake_token)
|
||||||
|
monkeypatch.setattr(webhook_common, "fetch_github_pr_metadata", fake_metadata)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
webhook_common,
|
||||||
|
"_get_thread_metadata_safe",
|
||||||
|
AsyncMock(return_value={"review_check_run_id": 77, "head_sha": "head-sha"}),
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
webhook_common, "_resolve_verdict_authorization", AsyncMock(return_value=True)
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", lambda *a, **k: None)
|
||||||
|
monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", AsyncMock())
|
||||||
|
monkeypatch.setattr(webhook_common, "create_review_check_run", created_check)
|
||||||
|
monkeypatch.setattr(webhook_common, "post_review_started_comment", AsyncMock(return_value=1))
|
||||||
|
monkeypatch.setattr(webhook_common, "_store_current_reviewer_run_id", AsyncMock())
|
||||||
|
monkeypatch.setattr(webhook_common, "get_client", lambda url: _FakeLangGraphClient())
|
||||||
|
|
||||||
|
result = asyncio.run(
|
||||||
|
github_webhooks.trigger_pr_review_from_ref(
|
||||||
|
GitHubPrRef(
|
||||||
|
owner="langchain-ai",
|
||||||
|
repo="open-swe",
|
||||||
|
number=1244,
|
||||||
|
url="https://github.com/langchain-ai/open-swe/pull/1244",
|
||||||
|
),
|
||||||
|
source="dashboard",
|
||||||
|
request_verdict=True,
|
||||||
|
)
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result["success"] is True
|
||||||
|
created_check.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None:
|
def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None:
|
||||||
captured: dict[str, object] = {}
|
captured: dict[str, object] = {}
|
||||||
|
|
||||||
|
|
@ -1183,7 +1261,9 @@ def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None
|
||||||
webhook_common, "_resolve_verdict_authorization", fake_resolve_verdict_authorization
|
webhook_common, "_resolve_verdict_authorization", fake_resolve_verdict_authorization
|
||||||
)
|
)
|
||||||
monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", lambda *a, **k: None)
|
monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", lambda *a, **k: None)
|
||||||
|
monkeypatch.setattr(webhook_common, "_get_thread_metadata_safe", AsyncMock(return_value={}))
|
||||||
monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", fake_async_noop)
|
monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", fake_async_noop)
|
||||||
|
monkeypatch.setattr(webhook_common, "create_review_check_run", fake_async_noop)
|
||||||
monkeypatch.setattr(webhook_common, "post_review_started_comment", fake_async_noop)
|
monkeypatch.setattr(webhook_common, "post_review_started_comment", fake_async_noop)
|
||||||
monkeypatch.setattr(webhook_common, "get_client", lambda url: _FakeLangGraphClient())
|
monkeypatch.setattr(webhook_common, "get_client", lambda url: _FakeLangGraphClient())
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -756,6 +756,7 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() ->
|
||||||
post_review = AsyncMock()
|
post_review = AsyncMock()
|
||||||
set_metadata = AsyncMock()
|
set_metadata = AsyncMock()
|
||||||
resolve_threads = AsyncMock(return_value=1)
|
resolve_threads = AsyncMock(return_value=1)
|
||||||
|
settle_check = AsyncMock()
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
|
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
|
||||||
|
|
@ -766,6 +767,7 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() ->
|
||||||
resolve_threads,
|
resolve_threads,
|
||||||
),
|
),
|
||||||
patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata),
|
patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata),
|
||||||
|
patch("agent.tools.publish_review.settle_review_check_run", settle_check),
|
||||||
patch(
|
patch(
|
||||||
"agent.tools.publish_review._maybe_post_slack_completion_reply",
|
"agent.tools.publish_review._maybe_post_slack_completion_reply",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -790,6 +792,8 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() ->
|
||||||
assert result["surfaced_count"] == 0
|
assert result["surfaced_count"] == 0
|
||||||
assert result["resolved_thread_count"] == 1
|
assert result["resolved_thread_count"] == 1
|
||||||
assert result["skipped_empty_re_review"] is True
|
assert result["skipped_empty_re_review"] is True
|
||||||
|
assert result["blocking_finding_count"] == 0
|
||||||
|
assert settle_check.await_args.kwargs["conclusion"] == "success"
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
|
|
@ -2539,6 +2543,7 @@ def _verdict_publish_patches(
|
||||||
post_review: AsyncMock,
|
post_review: AsyncMock,
|
||||||
thread_metadata: dict[str, Any] | None = _UNSET_METADATA,
|
thread_metadata: dict[str, Any] | None = _UNSET_METADATA,
|
||||||
dismiss: AsyncMock | None = None,
|
dismiss: AsyncMock | None = None,
|
||||||
|
settle_check: AsyncMock | None = None,
|
||||||
) -> list[Any]:
|
) -> list[Any]:
|
||||||
if thread_metadata is _UNSET_METADATA:
|
if thread_metadata is _UNSET_METADATA:
|
||||||
thread_metadata = {"pr": {"author": "external-contributor"}}
|
thread_metadata = {"pr": {"author": "external-contributor"}}
|
||||||
|
|
@ -2556,7 +2561,10 @@ def _verdict_publish_patches(
|
||||||
),
|
),
|
||||||
patch("agent.tools.publish_review.set_reviewer_thread_metadata", AsyncMock()),
|
patch("agent.tools.publish_review.set_reviewer_thread_metadata", AsyncMock()),
|
||||||
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
|
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
|
||||||
patch("agent.tools.publish_review.settle_review_check_run", AsyncMock()),
|
patch(
|
||||||
|
"agent.tools.publish_review.settle_review_check_run",
|
||||||
|
settle_check or AsyncMock(),
|
||||||
|
),
|
||||||
patch(
|
patch(
|
||||||
"agent.tools.publish_review.get_thread_metadata",
|
"agent.tools.publish_review.get_thread_metadata",
|
||||||
AsyncMock(return_value=thread_metadata or {}),
|
AsyncMock(return_value=thread_metadata or {}),
|
||||||
|
|
@ -2741,11 +2749,13 @@ async def test_publish_async_self_review_downgrades_verdict_to_comment() -> None
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
post_review = AsyncMock(return_value={"id": 1001, "state": "COMMENTED"})
|
post_review = AsyncMock(return_value={"id": 1001, "state": "COMMENTED"})
|
||||||
|
settle_check = AsyncMock()
|
||||||
with ExitStack() as stack:
|
with ExitStack() as stack:
|
||||||
for p in _verdict_publish_patches(
|
for p in _verdict_publish_patches(
|
||||||
findings=[],
|
findings=[],
|
||||||
post_review=post_review,
|
post_review=post_review,
|
||||||
thread_metadata={"pr": {"author": "seahaven-openswe[bot]"}},
|
thread_metadata={"pr": {"author": "seahaven-openswe[bot]"}},
|
||||||
|
settle_check=settle_check,
|
||||||
):
|
):
|
||||||
stack.enter_context(p)
|
stack.enter_context(p)
|
||||||
result = await _publish_review_async(
|
result = await _publish_review_async(
|
||||||
|
|
@ -2768,6 +2778,41 @@ async def test_publish_async_self_review_downgrades_verdict_to_comment() -> None
|
||||||
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
||||||
body = post_review.await_args.kwargs["body"]
|
body = post_review.await_args.kwargs["body"]
|
||||||
assert "Verdict withheld (self review)" in body
|
assert "Verdict withheld (self review)" in body
|
||||||
|
assert settle_check.await_args.kwargs["conclusion"] == "success"
|
||||||
|
|
||||||
|
|
||||||
|
async def test_publish_async_self_review_with_findings_fails_check() -> None:
|
||||||
|
from contextlib import ExitStack
|
||||||
|
|
||||||
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
|
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
|
||||||
|
post_review = AsyncMock(return_value={"id": 1002, "state": "COMMENTED"})
|
||||||
|
settle_check = AsyncMock()
|
||||||
|
with ExitStack() as stack:
|
||||||
|
for patcher in _verdict_publish_patches(
|
||||||
|
findings=findings,
|
||||||
|
post_review=post_review,
|
||||||
|
thread_metadata={"pr": {"author": "seahaven-openswe[bot]"}},
|
||||||
|
settle_check=settle_check,
|
||||||
|
):
|
||||||
|
stack.enter_context(patcher)
|
||||||
|
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,
|
||||||
|
verdict="request_changes",
|
||||||
|
verdict_authorization="consistent",
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result["verdict_ignored_reason"] == "self_review"
|
||||||
|
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
||||||
|
assert settle_check.await_args.kwargs["conclusion"] == "failure"
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_async_dismisses_stale_approval_on_new_findings() -> None:
|
async def test_publish_async_dismisses_stale_approval_on_new_findings() -> None:
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue