diff --git a/agent/webhooks/github.py b/agent/webhooks/github.py index 4258a092..776b9a61 100644 --- a/agent/webhooks/github.py +++ b/agent/webhooks/github.py @@ -1151,7 +1151,7 @@ _MIXED_REVIEW_ACTION_RE = re.compile( r"(?:^(?:please\s+)?|(?:^|[\s,;])(?:and|then|also)\s+(?:please\s+)?)" r"(?:add|address|change|commit|create|delete|deploy|fix|implement|merge|modify|" r"push|refactor|remove|resolve|run|update|write)\b", - re.IGNORECASE, + re.IGNORECASE | re.MULTILINE, ) diff --git a/tests/dashboard/test_public_repo_org_gate.py b/tests/dashboard/test_public_repo_org_gate.py index fcfe3d81..06c8b41c 100644 --- a/tests/dashboard/test_public_repo_org_gate.py +++ b/tests/dashboard/test_public_repo_org_gate.py @@ -56,11 +56,13 @@ def test_gate_blocks_non_member_on_public_pr_comment(monkeypatch) -> None: _common_setup(monkeypatch) seen = _install_membership_stub(monkeypatch, members={"insider"}) - async def fake_process_github_pr_comment(*_args, **_kwargs) -> None: + async def fake_process_github_review_command(*_args, **_kwargs) -> None: raise AssertionError("should not be called") monkeypatch.setattr( - github_webhooks, "process_github_pr_comment", fake_process_github_pr_comment + github_webhooks, + "process_github_review_command", + fake_process_github_review_command, ) client = TestClient(api_app.app) diff --git a/tests/github/test_review_comment_command.py b/tests/github/test_review_comment_command.py index 81580200..2a280bad 100644 --- a/tests/github/test_review_comment_command.py +++ b/tests/github/test_review_comment_command.py @@ -20,15 +20,22 @@ def _sign_body(body: bytes) -> str: return f"sha256={digest}" -def _post_issue_comment(client: TestClient, body_text: str) -> object: - payload = { - "action": "created", - "issue": { - "number": 245, - "html_url": "https://github.com/acme/widgets/pull/245", - "pull_request": {"html_url": "https://github.com/acme/widgets/pull/245"}, +def _post_webhook(client: TestClient, event_type: str, payload: dict[str, object]) -> object: + body = json.dumps(payload, separators=(",", ":")).encode() + return client.post( + "/webhooks/github", + content=body, + headers={ + "X-GitHub-Event": event_type, + "X-Hub-Signature-256": _sign_body(body), + "Content-Type": "application/json", }, - "comment": {"id": 91, "node_id": "IC_91", "body": body_text}, + ) + + +def _comment_payload(event_type: str, body_text: str) -> dict[str, object]: + payload: dict[str, object] = { + "action": "submitted" if event_type == "pull_request_review" else "created", "repository": { "owner": {"login": "acme"}, "name": "widgets", @@ -36,16 +43,25 @@ def _post_issue_comment(client: TestClient, body_text: str) -> object: }, "sender": {"login": "octocat", "id": 123}, } - body = json.dumps(payload, separators=(",", ":")).encode() - return client.post( - "/webhooks/github", - content=body, - headers={ - "X-GitHub-Event": "issue_comment", - "X-Hub-Signature-256": _sign_body(body), - "Content-Type": "application/json", - }, - ) + if event_type == "issue_comment": + payload["issue"] = { + "number": 245, + "html_url": "https://github.com/acme/widgets/pull/245", + "pull_request": {"html_url": "https://github.com/acme/widgets/pull/245"}, + } + payload["comment"] = {"id": 91, "node_id": "IC_91", "body": body_text} + else: + payload["pull_request"] = { + "number": 245, + "html_url": "https://github.com/acme/widgets/pull/245", + } + key = "review" if event_type == "pull_request_review" else "comment" + payload[key] = {"id": 91, "node_id": "IC_91", "body": body_text} + return payload + + +def _post_issue_comment(client: TestClient, body_text: str) -> object: + return _post_webhook(client, "issue_comment", _comment_payload("issue_comment", body_text)) @pytest.mark.parametrize( @@ -65,6 +81,10 @@ def _post_issue_comment(client: TestClient, body_text: str) -> object: "@openswe review\nPrioritize correctness over style.", "Prioritize correctness over style.", ), + ( + "@openswe review\nCheck authentication.\nValidate error handling.", + "Check authentication.\nValidate error handling.", + ), ], ) def test_parse_review_command(body: str, instructions: str) -> None: @@ -77,6 +97,7 @@ def test_parse_review_command(body: str, instructions: str) -> None: "@openswe review and fix the failing test", "@open-swe review focus on auth, then update the tests", "@openswe review fix the failing test", + "@openswe review Check authentication.\nFix the failing tests.", "Please @openswe review", "@openswe review this\n@openswe fix it", "@openswe reviewer", @@ -130,11 +151,100 @@ def test_review_command_route_bypasses_auto_review_and_coding_agent( assert captured["instructions"] == "Focus on the exact authorization boundary." -def test_mixed_review_request_falls_through_to_coding_agent( +@pytest.mark.parametrize( + "event_type", + ["pull_request_review", "pull_request_review_comment"], +) +def test_review_command_routes_review_event_payloads( + monkeypatch: pytest.MonkeyPatch, event_type: str +) -> None: + captured: dict[str, object] = {} + + async def allow_gate(*_args: object) -> None: + return None + + async def process_review( + payload: dict[str, object], received_event_type: str, *, instructions: str + ) -> None: + captured.update( + payload=payload, + event_type=received_event_type, + instructions=instructions, + ) + + async def fail_coding_agent(*_args: object, **_kwargs: object) -> None: + raise AssertionError("coding-agent dispatch must not run") + + monkeypatch.setattr(webhook_common, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) + monkeypatch.setattr(webhook_common, "_is_repo_allowed", lambda _repo: True) + monkeypatch.setattr(webhook_common, "_enforce_public_repo_org_gate", allow_gate) + monkeypatch.setattr(github_webhooks, "process_github_review_command", process_review) + monkeypatch.setattr(github_webhooks, "process_github_pr_comment", fail_coding_agent) + + response = _post_webhook( + TestClient(api_app.app), + event_type, + _comment_payload(event_type, "@open-swe review Focus on authorization."), + ) + + assert response.status_code == 200 + assert response.json() == { + "status": "accepted", + "message": "Processing on-demand PR review", + } + assert captured["event_type"] == event_type + assert captured["instructions"] == "Focus on authorization." + + +def test_finding_reply_takes_precedence_over_review_command( monkeypatch: pytest.MonkeyPatch, ) -> None: captured: dict[str, object] = {} + async def allow_gate(*_args: object) -> None: + return None + + async def process_finding_reply(payload: dict[str, object]) -> None: + captured["payload"] = payload + + async def fail_review(*_args: object, **_kwargs: object) -> None: + raise AssertionError("review command must not steal finding replies") + + payload = _comment_payload("pull_request_review_comment", "@openswe review") + comment = payload["comment"] + assert isinstance(comment, dict) + comment["in_reply_to_id"] = 90 + + monkeypatch.setattr(webhook_common, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) + monkeypatch.setattr(webhook_common, "_is_repo_allowed", lambda _repo: True) + monkeypatch.setattr(webhook_common, "_enforce_public_repo_org_gate", allow_gate) + monkeypatch.setattr( + github_webhooks, "process_github_review_finding_reply", process_finding_reply + ) + monkeypatch.setattr(github_webhooks, "process_github_review_command", fail_review) + + response = _post_webhook(TestClient(api_app.app), "pull_request_review_comment", payload) + + assert response.status_code == 200 + assert response.json() == { + "status": "accepted", + "message": "Processing review finding reply", + } + assert captured["payload"] == payload + + +@pytest.mark.parametrize( + "body", + [ + "@openswe review and fix the failing test", + "@openswe review Check authentication.\nFix the failing tests.", + ], +) +def test_mixed_review_request_falls_through_to_coding_agent( + monkeypatch: pytest.MonkeyPatch, body: str +) -> None: + captured: dict[str, object] = {} + async def allow_gate(*_args: object) -> None: return None @@ -150,9 +260,7 @@ def test_mixed_review_request_falls_through_to_coding_agent( monkeypatch.setattr(github_webhooks, "process_github_review_command", fail_review) monkeypatch.setattr(github_webhooks, "process_github_pr_comment", process_coding_agent) - response = _post_issue_comment( - TestClient(api_app.app), "@openswe review and fix the failing test" - ) + response = _post_issue_comment(TestClient(api_app.app), body) assert response.status_code == 200 assert response.json()["status"] == "accepted"