From 2f05fe56065150be768907d1a349694b8d69a5a7 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 13 Apr 2026 12:43:52 +0000 Subject: [PATCH 1/3] feat: add AI code review integration (#1067) Add thin HTTP client that calls the external pr-ai-reviewer service for AI-powered code review on PRs. - New ai-review top-level config (server-url, providers, triggers) - Thin HTTP client in webhook_server/libs/ai_review.py (~100 lines) - Fire-and-forget on PR opened and synchronize (non-clean rebase) - Same pattern as test-oracle integration - 15 new tests, all 1508 tests pass, 90.67% coverage Closes #1067 --- CLAUDE.md | 23 ++ examples/config.yaml | 17 ++ webhook_server/config/schema.yaml | 134 +++++++++++ webhook_server/libs/ai_review.py | 120 ++++++++++ webhook_server/libs/github_api.py | 3 + .../libs/handlers/pull_request_handler.py | 23 ++ webhook_server/tests/test_ai_review.py | 219 ++++++++++++++++++ .../tests/test_clean_rebase_detection.py | 1 + .../tests/test_pull_request_handler.py | 131 +++++++++++ webhook_server/tests/test_runner_handler.py | 3 + 10 files changed, 674 insertions(+) create mode 100644 webhook_server/libs/ai_review.py create mode 100644 webhook_server/tests/test_ai_review.py diff --git a/CLAUDE.md b/CLAUDE.md index 460417e2..ac5b70ca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -682,3 +682,26 @@ AI-powered enhancements controlled by `ai-features` config (global or per-repo). **On AI CLI failure:** Error is logged, flow continues without suggestion **Module:** `webhook_server/libs/ai_cli.py` - shared AI CLI wrapper + +### AI Code Review Integration + +External AI service integration for PR code review via [pr-ai-reviewer](https://github.com/myk-org/pr-ai-reviewer). + +**Schema:** `webhook_server/config/schema.yaml` (`ai-review`), configurable globally or per-repo + +**Triggers:** `pr-opened`, `pr-synchronized` (skips clean rebases). Default: both enabled. + +**Module:** `webhook_server/libs/ai_review.py` - `call_ai_reviewer()` thin HTTP client + +**Features:** + +- 3 specialized review agents (quality, guidelines, security) from [pi-config](https://github.com/myk-org/pi-config) +- Single provider: standard AI review +- Multiple providers: peer review with consensus loop +- Posts inline review comments on PR diff lines via GitHub Pull Request Review API + +**Error handling:** + +- Health check failure: PR comment posted, continue flow +- Review errors: log only, no PR comment +- Never breaks webhook processing diff --git a/examples/config.yaml b/examples/config.yaml index 4be978e6..437d9bb3 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -123,6 +123,23 @@ test-oracle: # - pr-opened # Run when PR is opened # - pr-synchronized # Run when new commits pushed +# AI Code Review integration +# Reviews PR diffs using specialized AI review agents +# See: https://github.com/myk-org/pr-ai-reviewer +ai-review: + server-url: "http://localhost:8001" + providers: + - ai-provider: "claude" + ai-model: "claude-sonnet-4-20250514" + # Add more providers for peer review: + # - ai-provider: "gemini" + # ai-model: "gemini-2.5-pro" + max-rounds: 3 # Max peer review rounds (default: 3) + timeout-minutes: 30 # Timeout per AI CLI call (default: 30) + triggers: # Default: [pr-opened, pr-synchronized] + - pr-opened + - pr-synchronized + # AI Features configuration # Enables AI-powered enhancements (e.g., conventional title suggestions) ai-features: diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index 78565127..52ae6a78 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -213,6 +213,73 @@ properties: additionalProperties: false ai-features: $ref: '#/$defs/ai-features' + ai-review: + type: object + description: | + AI-powered code review for pull requests. + See https://github.com/myk-org/pr-ai-reviewer for server setup. + Each provider uses 3 specialized review agents (quality, guidelines, security). + Single provider: standard review. + Multiple providers: peer review (first = main, rest = peers). + They debate until consensus, then final comments are posted. + properties: + server-url: + type: string + format: uri + description: URL of the pr-ai-reviewer server (e.g., http://localhost:8001) + providers: + type: array + description: | + List of AI providers for code review. + Single provider: standard review. + Multiple providers: peer review (first = main reviewer, rest = peers). + items: + type: object + properties: + ai-provider: + type: string + enum: + - claude + - gemini + - cursor + description: AI CLI provider to use for review + ai-model: + type: string + description: AI model identifier + required: + - ai-provider + - ai-model + additionalProperties: false + minItems: 1 + max-rounds: + type: integer + minimum: 1 + maximum: 5 + default: 3 + description: Maximum peer review debate rounds before accepting main AI result + timeout-minutes: + type: integer + minimum: 1 + default: 30 + description: Timeout in minutes for each AI CLI call + triggers: + type: array + default: + - pr-opened + - pr-synchronized + items: + type: string + enum: + - pr-opened + - pr-synchronized + description: | + When to automatically run AI code review. Default: [pr-opened, pr-synchronized]. + - pr-opened: Run when a new PR is opened + - pr-synchronized: Run when new commits are pushed to a PR (skips clean rebases) + required: + - server-url + - providers + additionalProperties: false labels: type: object description: Configure which labels are enabled and their colors @@ -577,6 +644,73 @@ properties: additionalProperties: false ai-features: $ref: '#/$defs/ai-features' + ai-review: + type: object + description: | + AI-powered code review for pull requests. + See https://github.com/myk-org/pr-ai-reviewer for server setup. + Each provider uses 3 specialized review agents (quality, guidelines, security). + Single provider: standard review. + Multiple providers: peer review (first = main, rest = peers). + They debate until consensus, then final comments are posted. + properties: + server-url: + type: string + format: uri + description: URL of the pr-ai-reviewer server (e.g., http://localhost:8001) + providers: + type: array + description: | + List of AI providers for code review. + Single provider: standard review. + Multiple providers: peer review (first = main reviewer, rest = peers). + items: + type: object + properties: + ai-provider: + type: string + enum: + - claude + - gemini + - cursor + description: AI CLI provider to use for review + ai-model: + type: string + description: AI model identifier + required: + - ai-provider + - ai-model + additionalProperties: false + minItems: 1 + max-rounds: + type: integer + minimum: 1 + maximum: 5 + default: 3 + description: Maximum peer review debate rounds before accepting main AI result + timeout-minutes: + type: integer + minimum: 1 + default: 30 + description: Timeout in minutes for each AI CLI call + triggers: + type: array + default: + - pr-opened + - pr-synchronized + items: + type: string + enum: + - pr-opened + - pr-synchronized + description: | + When to automatically run AI code review. Default: [pr-opened, pr-synchronized]. + - pr-opened: Run when a new PR is opened + - pr-synchronized: Run when new commits are pushed to a PR (skips clean rebases) + required: + - server-url + - providers + additionalProperties: false custom-check-runs: type: array description: | diff --git a/webhook_server/libs/ai_review.py b/webhook_server/libs/ai_review.py new file mode 100644 index 00000000..e5c860ef --- /dev/null +++ b/webhook_server/libs/ai_review.py @@ -0,0 +1,120 @@ +from __future__ import annotations + +import asyncio +from typing import TYPE_CHECKING, Any + +import httpx + +if TYPE_CHECKING: + from github.PullRequest import PullRequest + + from webhook_server.libs.github_api import GithubWebhook + +DEFAULT_TRIGGERS: list[str] = ["pr-opened", "pr-synchronized"] + + +async def call_ai_reviewer( + github_webhook: GithubWebhook, + pull_request: PullRequest, + trigger: str | None = None, +) -> None: + """Call the pr-ai-reviewer service to review a PR. + + Args: + github_webhook: The GithubWebhook instance with config and token. + pull_request: The PyGithub PullRequest object. + trigger: The event trigger (e.g., "pr-opened", "pr-synchronized"). + None means command-triggered (always runs if configured). + Command trigger is reserved for a future /ai-review comment command. + """ + config = github_webhook.ai_review_config + if not config: + return + + # trigger=None means command-triggered (e.g., future /ai-review command). + # Currently only webhook-triggered via pr-opened/pr-synchronized. + + if trigger is not None: + triggers: list[str] = config.get("triggers", DEFAULT_TRIGGERS) + if trigger not in triggers: + github_webhook.logger.debug( + f"{github_webhook.log_prefix} AI reviewer trigger '{trigger}' not in configured triggers {triggers}" + ) + return + + server_url: str = config["server-url"] + log_prefix: str = github_webhook.log_prefix + + try: + async with httpx.AsyncClient(base_url=server_url) as client: + # Health check + try: + health_response = await client.get("/health", timeout=5.0) + health_response.raise_for_status() + except httpx.HTTPError as e: + status_info = "" + if isinstance(e, httpx.HTTPStatusError): + status_info = f" (status {e.response.status_code})" + + msg = f"AI Reviewer server at {server_url} is not responding{status_info}, skipping code review" + github_webhook.logger.warning(f"{log_prefix} {msg}") + try: + await asyncio.to_thread( + pull_request.create_issue_comment, + f"AI Reviewer server is not responding{status_info}, skipping code review", + ) + except Exception: + github_webhook.logger.exception(f"{log_prefix} Failed to post health check comment") + return + + # Build review payload + pr_url: str = await asyncio.to_thread(lambda: pull_request.html_url) + + # Convert provider configs from YAML format to API format + providers_config: list[dict[str, str]] = config.get("providers", []) + providers_payload: list[dict[str, str]] = [ + {"ai_provider": p["ai-provider"], "ai_model": p["ai-model"]} for p in providers_config + ] + + payload: dict[str, Any] = { + "pr_url": pr_url, + "providers": providers_payload, + "github_token": github_webhook.token, + } + + if "max-rounds" in config: + payload["max_rounds"] = config["max-rounds"] + + if "timeout-minutes" in config: + payload["timeout_minutes"] = config["timeout-minutes"] + + # Call review endpoint + try: + github_webhook.logger.info(f"{log_prefix} Calling AI Reviewer for {pr_url}") + # Long timeout: AI review with peer consensus can take up to timeout_minutes * max_rounds + timeout_minutes = config.get("timeout-minutes", 30) + max_rounds = config.get("max-rounds", 3) + # Worst-case: each round takes timeout_minutes (sequential AI calls) + 60s buffer + request_timeout = float(timeout_minutes * max_rounds * 60 + 60) + + response = await client.post("/review", json=payload, timeout=request_timeout) + response.raise_for_status() + + result = response.json() + review_posted = result.get("review_posted", False) + comments_count = len(result.get("comments", [])) + summary = result.get("summary", "no summary") + + github_webhook.logger.info( + f"{log_prefix} AI Reviewer complete: {comments_count} comment(s), " + f"review_posted={review_posted}, summary={summary}" + ) + except httpx.HTTPError as e: + err_detail = f": {e.response.text}" if isinstance(e, httpx.HTTPStatusError) else "" + github_webhook.logger.error(f"{log_prefix} AI Reviewer request failed{err_detail}") + except ValueError: + github_webhook.logger.error(f"{log_prefix} AI Reviewer returned invalid JSON response") + except asyncio.CancelledError: + raise + except Exception: + github_webhook.logger.exception(f"{log_prefix} AI Reviewer call failed unexpectedly") diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 2aa76044..ccf1a34e 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -808,6 +808,9 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: self.ai_features: dict[str, Any] | None = self.config.get_value( value="ai-features", return_on_none=None, extra_dict=repository_config ) + self.ai_review_config: dict[str, Any] | None = self.config.get_value( + value="ai-review", return_on_none=None, extra_dict=repository_config + ) _auto_merge_prs = self.config.get_value( value="set-auto-merge-prs", return_on_none=[], extra_dict=repository_config ) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index fd844642..f1b2dfee 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -12,6 +12,7 @@ from github.Repository import Repository from timeout_sampler import TimeoutExpiredError, TimeoutSampler +from webhook_server.libs.ai_review import call_ai_reviewer from webhook_server.libs.handlers.check_run_handler import CheckRunHandler, CheckRunOutput from webhook_server.libs.handlers.labels_handler import LabelsHandler from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler @@ -275,6 +276,17 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> _background_tasks.add(task) task.add_done_callback(_background_tasks.discard) + if hook_action == "opened" and self.github_webhook.ai_review_config: + ai_review_task = asyncio.create_task( + call_ai_reviewer( + github_webhook=self.github_webhook, + pull_request=pull_request, + trigger="pr-opened", + ) + ) + _background_tasks.add(ai_review_task) + ai_review_task.add_done_callback(_background_tasks.discard) + if self.ctx: self.ctx.complete_step("pr_handler", action=hook_action) return @@ -315,6 +327,17 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> _background_tasks.add(task) task.add_done_callback(_background_tasks.discard) + if not clean_rebase and self.github_webhook.ai_review_config: + ai_review_task = asyncio.create_task( + call_ai_reviewer( + github_webhook=self.github_webhook, + pull_request=pull_request, + trigger="pr-synchronized", + ) + ) + _background_tasks.add(ai_review_task) + ai_review_task.add_done_callback(_background_tasks.discard) + if self.ctx: self.ctx.complete_step("pr_handler", action=hook_action) return diff --git a/webhook_server/tests/test_ai_review.py b/webhook_server/tests/test_ai_review.py new file mode 100644 index 00000000..be2d7354 --- /dev/null +++ b/webhook_server/tests/test_ai_review.py @@ -0,0 +1,219 @@ +"""Tests for webhook_server.libs.ai_review module.""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock, Mock, patch + +import httpx +import pytest + +from webhook_server.libs.ai_review import DEFAULT_TRIGGERS, call_ai_reviewer + + +@pytest.fixture +def mock_github_webhook() -> Mock: + """Create a mock GithubWebhook for AI review tests.""" + mock = Mock() + mock.ai_review_config = { + "server-url": "http://localhost:8001", + "providers": [ + {"ai-provider": "claude", "ai-model": "sonnet"}, + ], + "triggers": ["pr-opened", "pr-synchronized"], + } + mock.logger = MagicMock() + mock.log_prefix = "[TEST]" + mock.token = "ghp_test_token" # pragma: allowlist secret + return mock + + +@pytest.fixture +def mock_pull_request() -> Mock: + """Create a mock PullRequest.""" + mock = Mock() + mock.html_url = "https://github.com/test-org/test-repo/pull/42" + mock.create_issue_comment = Mock() + return mock + + +class TestCallAiReviewer: + @pytest.mark.asyncio + async def test_returns_early_when_config_is_none(self, mock_pull_request: Mock) -> None: + mock_webhook = Mock() + mock_webhook.ai_review_config = None + await call_ai_reviewer(mock_webhook, mock_pull_request) + mock_webhook.logger.debug.assert_not_called() + + @pytest.mark.asyncio + async def test_returns_early_when_trigger_not_configured( + self, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + mock_github_webhook.ai_review_config["triggers"] = ["pr-opened"] + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-synchronized") + mock_github_webhook.logger.debug.assert_called_once() + + @pytest.mark.asyncio + async def test_command_trigger_always_runs(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + """When trigger is None (command-triggered), always run regardless of configured triggers.""" + mock_response = Mock() + mock_response.status_code = 200 + mock_response.raise_for_status = Mock() + mock_response.json.return_value = {"status": "healthy"} + + review_response = Mock() + review_response.status_code = 200 + review_response.raise_for_status = Mock() + review_response.json.return_value = { + "review_posted": True, + "comments": [{"path": "a.py", "line": 1, "body": "issue"}], + "summary": "1 issue found", + } + + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + mock_client.get = AsyncMock(return_value=mock_response) + mock_client.post = AsyncMock(return_value=review_response) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger=None) + + mock_client.post.assert_called_once() + + @pytest.mark.asyncio + async def test_health_check_failure_posts_comment(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + mock_client.get = AsyncMock(side_effect=httpx.ConnectError("Connection refused")) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + + mock_github_webhook.logger.warning.assert_called_once() + mock_pull_request.create_issue_comment.assert_called_once() + + @pytest.mark.asyncio + async def test_successful_review(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + health_response = Mock() + health_response.raise_for_status = Mock() + + review_response = Mock() + review_response.raise_for_status = Mock() + review_response.json.return_value = { + "review_posted": True, + "comments": [{"path": "a.py", "line": 1, "body": "bug"}], + "summary": "1 issue", + } + + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + mock_client.get = AsyncMock(return_value=health_response) + mock_client.post = AsyncMock(return_value=review_response) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + + mock_github_webhook.logger.info.assert_called() + + @pytest.mark.asyncio + async def test_review_http_error(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + health_response = Mock() + health_response.raise_for_status = Mock() + + error_response = Mock() + error_response.status_code = 502 + error_response.text = "Bad Gateway" + review_error = httpx.HTTPStatusError("error", request=Mock(), response=error_response) + + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + mock_client.get = AsyncMock(return_value=health_response) + mock_client.post = AsyncMock(side_effect=review_error) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + + mock_github_webhook.logger.error.assert_called_once() + + @pytest.mark.asyncio + async def test_review_invalid_json(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + health_response = Mock() + health_response.raise_for_status = Mock() + + review_response = Mock() + review_response.raise_for_status = Mock() + review_response.json.side_effect = ValueError("Invalid JSON") + + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + mock_client.get = AsyncMock(return_value=health_response) + mock_client.post = AsyncMock(return_value=review_response) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + + mock_github_webhook.logger.error.assert_called_once() + + @pytest.mark.asyncio + async def test_unexpected_exception(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + with patch("webhook_server.libs.ai_review.httpx.AsyncClient", side_effect=RuntimeError("boom")): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + mock_github_webhook.logger.exception.assert_called_once() + + @pytest.mark.asyncio + async def test_cancelled_error_reraised(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + with patch("webhook_server.libs.ai_review.httpx.AsyncClient", side_effect=asyncio.CancelledError): + with pytest.raises(asyncio.CancelledError): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + + @pytest.mark.asyncio + async def test_provider_config_conversion(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + """Verify YAML-format providers are converted to API format.""" + mock_github_webhook.ai_review_config["providers"] = [ + {"ai-provider": "claude", "ai-model": "sonnet"}, + {"ai-provider": "gemini", "ai-model": "pro"}, + ] + + health_response = Mock() + health_response.raise_for_status = Mock() + + review_response = Mock() + review_response.raise_for_status = Mock() + review_response.json.return_value = {"review_posted": False, "comments": [], "summary": "ok"} + + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + mock_client.get = AsyncMock(return_value=health_response) + mock_client.post = AsyncMock(return_value=review_response) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + + # Check the payload sent to the service + call_args = mock_client.post.call_args + payload = call_args.kwargs.get("json") or call_args[1].get("json") + assert payload["providers"] == [ + {"ai_provider": "claude", "ai_model": "sonnet"}, + {"ai_provider": "gemini", "ai_model": "pro"}, + ] + + +class TestDefaultTriggers: + def test_default_triggers(self) -> None: + assert DEFAULT_TRIGGERS == ["pr-opened", "pr-synchronized"] diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index 51cb9d03..520fb45a 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -73,6 +73,7 @@ def mock_github_webhook() -> Mock: mock_webhook.enabled_labels = None mock_webhook.custom_check_runs = [] mock_webhook.ai_features = None + mock_webhook.ai_review_config = None mock_webhook.required_conversation_resolution = False mock_webhook.config = Mock() mock_webhook.config.get_value = Mock(return_value=None) diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 0f2263d0..7fc8dff5 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -93,6 +93,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.enabled_labels = None # Default: all labels enabled mock_webhook.custom_check_runs = [] mock_webhook.ai_features = None + mock_webhook.ai_review_config = None mock_webhook.required_conversation_resolution = False mock_webhook.config = Mock() mock_webhook.config.get_value = Mock(return_value=None) @@ -476,6 +477,7 @@ def test_prepare_welcome_comment_issue_creation_disabled(self, pull_request_hand def test_prepare_ai_features_section_no_features(self, pull_request_handler: PullRequestHandler) -> None: """Test AI features section is empty when no features are configured.""" pull_request_handler.github_webhook.ai_features = None + pull_request_handler.github_webhook.ai_review_config = None result = pull_request_handler._prepare_ai_features_welcome_section assert result == "" @@ -3285,3 +3287,132 @@ async def test_process_reopened_action_does_not_call_test_oracle( await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) mock_test_oracle.assert_not_called() mock_create_task.assert_not_called() + + @pytest.mark.asyncio + async def test_ai_review_triggered_on_opened( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that call_ai_reviewer is fired as a background task with trigger='pr-opened' when a PR is opened.""" + pull_request_handler.hook_data["action"] = "opened" + pull_request_handler.github_webhook.ai_review_config = { + "server-url": "http://localhost:8001", + "providers": [{"ai-provider": "claude", "ai-model": "sonnet"}], + } + + with ( + patch.object(pull_request_handler, "create_issue_for_new_pull_request"), + patch.object(pull_request_handler, "set_wip_label_based_on_title"), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request"), + patch.object(pull_request_handler, "set_pull_request_automerge"), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_ai_reviewer", + new_callable=AsyncMock, + ) as mock_ai_reviewer, + patch("asyncio.create_task") as mock_create_task, + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + mock_ai_reviewer.assert_called_once_with( + github_webhook=pull_request_handler.github_webhook, + pull_request=mock_pull_request, + trigger="pr-opened", + ) + assert mock_create_task.call_count == 2 + + @pytest.mark.asyncio + async def test_ai_review_not_triggered_when_config_none( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that call_ai_reviewer is NOT called when ai_review_config is None.""" + pull_request_handler.hook_data["action"] = "opened" + pull_request_handler.github_webhook.ai_review_config = None + + with ( + patch.object(pull_request_handler, "create_issue_for_new_pull_request"), + patch.object(pull_request_handler, "set_wip_label_based_on_title"), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request"), + patch.object(pull_request_handler, "set_pull_request_automerge"), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_ai_reviewer", + new_callable=AsyncMock, + ) as mock_ai_reviewer, + patch("asyncio.create_task") as mock_create_task, + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + mock_ai_reviewer.assert_not_called() + # Only test_oracle should create a task, not ai_reviewer + mock_create_task.assert_called_once() + + @pytest.mark.asyncio + async def test_ai_review_triggered_on_synchronize_non_clean( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that call_ai_reviewer is fired with trigger='pr-synchronized' on non-clean-rebase synchronize.""" + pull_request_handler.hook_data["action"] = "synchronize" + pull_request_handler.hook_data["before"] = "aaa1111111111111111111111111111111111111" + pull_request_handler.hook_data["after"] = "bbb2222222222222222222222222222222222222" + pull_request_handler.github_webhook.ai_review_config = { + "server-url": "http://localhost:8001", + "providers": [{"ai-provider": "claude", "ai-model": "sonnet"}], + } + + with ( + patch.object(pull_request_handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=False), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request"), + patch.object(pull_request_handler, "remove_labels_when_pull_request_sync"), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_ai_reviewer", + new_callable=AsyncMock, + ) as mock_ai_reviewer, + patch("asyncio.create_task") as mock_create_task, + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + mock_ai_reviewer.assert_called_once_with( + github_webhook=pull_request_handler.github_webhook, + pull_request=mock_pull_request, + trigger="pr-synchronized", + ) + assert mock_create_task.call_count == 2 + + @pytest.mark.asyncio + async def test_ai_review_not_triggered_on_clean_rebase( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that call_ai_reviewer is NOT called when synchronize is a clean rebase.""" + pull_request_handler.hook_data["action"] = "synchronize" + pull_request_handler.hook_data["before"] = "aaa1111111111111111111111111111111111111" + pull_request_handler.hook_data["after"] = "bbb2222222222222222222222222222222222222" + pull_request_handler.github_webhook.ai_review_config = { + "server-url": "http://localhost:8001", + "providers": [{"ai-provider": "claude", "ai-model": "sonnet"}], + } + + with ( + patch.object(pull_request_handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(pull_request_handler, "_post_clean_rebase_comment", new_callable=AsyncMock), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request"), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_ai_reviewer", + new_callable=AsyncMock, + ) as mock_ai_reviewer, + patch("asyncio.create_task") as mock_create_task, + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + mock_ai_reviewer.assert_not_called() + # Only test_oracle should create a task, not ai_reviewer + mock_create_task.assert_called_once() diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 203ede24..09736785 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -57,6 +57,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.ctx = None mock_webhook.custom_check_runs = [] mock_webhook.ai_features = None + mock_webhook.ai_review_config = None mock_webhook.config = Mock() mock_webhook.config.get_value = Mock(return_value=None) return mock_webhook @@ -754,6 +755,7 @@ async def test_conventional_title_failure_without_ai_features( runner_handler.github_webhook.conventional_title = "feat,fix" mock_pull_request.title = "bad title" runner_handler.github_webhook.ai_features = None + runner_handler.github_webhook.ai_review_config = None with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) @@ -1811,6 +1813,7 @@ async def test_cherry_pick_ai_not_configured_fallback( ) -> None: """Cherry-pick conflicts + AI not configured — manual fallback, call_ai_cli not called.""" runner_handler.github_webhook.ai_features = None + runner_handler.github_webhook.ai_review_config = None async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "cherry-pick" in command and "--continue" not in command: From afda2a1b1010e7e5085a1695b60bebc12b01ee38 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 13 Apr 2026 13:31:12 +0000 Subject: [PATCH 2/3] chore: checkpoint before acpx changes --- examples/config.yaml | 12 +- webhook_server/libs/ai_review.py | 46 ++++-- .../libs/handlers/pull_request_handler.py | 6 + webhook_server/tests/test_ai_review.py | 142 ++++++++++++++---- .../tests/test_pull_request_handler.py | 2 + webhook_server/utils/constants.py | 1 + 6 files changed, 163 insertions(+), 46 deletions(-) diff --git a/examples/config.yaml b/examples/config.yaml index 437d9bb3..b936b177 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -130,13 +130,13 @@ ai-review: server-url: "http://localhost:8001" providers: - ai-provider: "claude" - ai-model: "claude-sonnet-4-20250514" + ai-model: "claude-opus-4-6[1m]" # Add more providers for peer review: # - ai-provider: "gemini" # ai-model: "gemini-2.5-pro" - max-rounds: 3 # Max peer review rounds (default: 3) - timeout-minutes: 30 # Timeout per AI CLI call (default: 30) - triggers: # Default: [pr-opened, pr-synchronized] + max-rounds: 3 # Max peer review rounds (default: 3) + timeout-minutes: 30 # Timeout per AI CLI call (default: 30) + triggers: # Default: [pr-opened, pr-synchronized] - pr-opened - pr-synchronized @@ -147,11 +147,11 @@ ai-features: ai-model: "claude-opus-4-6[1m]" conventional-title: enabled: true - mode: suggest # suggest: show in checkrun | fix: auto-update PR title + mode: suggest # suggest: show in checkrun | fix: auto-update PR title timeout-minutes: 10 resolve-cherry-pick-conflicts-with-ai: enabled: true - timeout-minutes: 10 # Timeout in minutes for AI CLI (default: 10) + timeout-minutes: 10 # Timeout in minutes for AI CLI (default: 10) repositories: my-repository: diff --git a/webhook_server/libs/ai_review.py b/webhook_server/libs/ai_review.py index e5c860ef..fc22b0fe 100644 --- a/webhook_server/libs/ai_review.py +++ b/webhook_server/libs/ai_review.py @@ -5,10 +5,13 @@ import httpx +from webhook_server.utils.constants import AI_REVIEW_STR + if TYPE_CHECKING: from github.PullRequest import PullRequest from webhook_server.libs.github_api import GithubWebhook + from webhook_server.libs.handlers.check_run_handler import CheckRunHandler DEFAULT_TRIGGERS: list[str] = ["pr-opened", "pr-synchronized"] @@ -16,6 +19,7 @@ async def call_ai_reviewer( github_webhook: GithubWebhook, pull_request: PullRequest, + check_run_handler: CheckRunHandler, trigger: str | None = None, ) -> None: """Call the pr-ai-reviewer service to review a PR. @@ -23,6 +27,7 @@ async def call_ai_reviewer( Args: github_webhook: The GithubWebhook instance with config and token. pull_request: The PyGithub PullRequest object. + check_run_handler: CheckRunHandler for updating check run status. trigger: The event trigger (e.g., "pr-opened", "pr-synchronized"). None means command-triggered (always runs if configured). Command trigger is reserved for a future /ai-review comment command. @@ -46,6 +51,8 @@ async def call_ai_reviewer( log_prefix: str = github_webhook.log_prefix try: + await check_run_handler.set_check_in_progress(name=AI_REVIEW_STR) + async with httpx.AsyncClient(base_url=server_url) as client: # Health check try: @@ -56,15 +63,12 @@ async def call_ai_reviewer( if isinstance(e, httpx.HTTPStatusError): status_info = f" (status {e.response.status_code})" - msg = f"AI Reviewer server at {server_url} is not responding{status_info}, skipping code review" + msg = f"AI Reviewer server at {server_url} is not responding{status_info}" github_webhook.logger.warning(f"{log_prefix} {msg}") - try: - await asyncio.to_thread( - pull_request.create_issue_comment, - f"AI Reviewer server is not responding{status_info}, skipping code review", - ) - except Exception: - github_webhook.logger.exception(f"{log_prefix} Failed to post health check comment") + await check_run_handler.set_check_failure( + name=AI_REVIEW_STR, + output={"title": "AI Review Failed", "summary": msg}, + ) return # Build review payload @@ -109,12 +113,36 @@ async def call_ai_reviewer( f"{log_prefix} AI Reviewer complete: {comments_count} comment(s), " f"review_posted={review_posted}, summary={summary}" ) + + await check_run_handler.set_check_success( + name=AI_REVIEW_STR, + output={ + "title": "AI Review Complete", + "summary": f"{comments_count} comment(s) posted" if comments_count else "No issues found", + }, + ) except httpx.HTTPError as e: err_detail = f": {e.response.text}" if isinstance(e, httpx.HTTPStatusError) else "" - github_webhook.logger.error(f"{log_prefix} AI Reviewer request failed{err_detail}") + error_msg = f"AI Reviewer request failed{err_detail}" + github_webhook.logger.error(f"{log_prefix} {error_msg}") + await check_run_handler.set_check_failure( + name=AI_REVIEW_STR, + output={"title": "AI Review Failed", "summary": error_msg}, + ) except ValueError: github_webhook.logger.error(f"{log_prefix} AI Reviewer returned invalid JSON response") + await check_run_handler.set_check_failure( + name=AI_REVIEW_STR, + output={"title": "AI Review Failed", "summary": "Invalid JSON response from AI Reviewer"}, + ) except asyncio.CancelledError: raise except Exception: github_webhook.logger.exception(f"{log_prefix} AI Reviewer call failed unexpectedly") + try: + await check_run_handler.set_check_failure( + name=AI_REVIEW_STR, + output={"title": "AI Review Failed", "summary": "Unexpected error"}, + ) + except Exception: + github_webhook.logger.exception(f"{log_prefix} Failed to set check run failure") diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index f1b2dfee..3d557781 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -20,6 +20,7 @@ from webhook_server.libs.test_oracle import call_test_oracle from webhook_server.utils.constants import ( AI_RESOLVED_CONFLICTS_LABEL, + AI_REVIEW_STR, APPROVED_BY_LABEL_PREFIX, AUTOMERGE_LABEL_STR, BRANCH_LABEL_PREFIX, @@ -281,6 +282,7 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> call_ai_reviewer( github_webhook=self.github_webhook, pull_request=pull_request, + check_run_handler=self.check_run_handler, trigger="pr-opened", ) ) @@ -332,6 +334,7 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> call_ai_reviewer( github_webhook=self.github_webhook, pull_request=pull_request, + check_run_handler=self.check_run_handler, trigger="pr-synchronized", ) ) @@ -1065,6 +1068,9 @@ async def process_opened_or_synchronize_pull_request( if self.github_webhook.build_and_push_container: setup_tasks.append(self.check_run_handler.set_check_queued(name=BUILD_CONTAINER_STR)) + if self.github_webhook.ai_review_config: + setup_tasks.append(self.check_run_handler.set_check_queued(name=AI_REVIEW_STR)) + if is_clean_rebase: # label_names is guaranteed non-None when is_clean_rebase=True (caller always provides it) setup_tasks.append( diff --git a/webhook_server/tests/test_ai_review.py b/webhook_server/tests/test_ai_review.py index be2d7354..6021eb4f 100644 --- a/webhook_server/tests/test_ai_review.py +++ b/webhook_server/tests/test_ai_review.py @@ -7,6 +7,7 @@ import pytest from webhook_server.libs.ai_review import DEFAULT_TRIGGERS, call_ai_reviewer +from webhook_server.utils.constants import AI_REVIEW_STR @pytest.fixture @@ -31,36 +32,50 @@ def mock_pull_request() -> Mock: """Create a mock PullRequest.""" mock = Mock() mock.html_url = "https://github.com/test-org/test-repo/pull/42" - mock.create_issue_comment = Mock() + return mock + + +@pytest.fixture +def mock_check_run_handler() -> AsyncMock: + """Create a mock CheckRunHandler.""" + mock = AsyncMock() + mock.set_check_in_progress = AsyncMock() + mock.set_check_success = AsyncMock() + mock.set_check_failure = AsyncMock() return mock class TestCallAiReviewer: @pytest.mark.asyncio - async def test_returns_early_when_config_is_none(self, mock_pull_request: Mock) -> None: + async def test_returns_early_when_config_is_none( + self, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: mock_webhook = Mock() mock_webhook.ai_review_config = None - await call_ai_reviewer(mock_webhook, mock_pull_request) - mock_webhook.logger.debug.assert_not_called() + await call_ai_reviewer(mock_webhook, mock_pull_request, mock_check_run_handler) + mock_check_run_handler.set_check_in_progress.assert_not_called() @pytest.mark.asyncio async def test_returns_early_when_trigger_not_configured( - self, mock_github_webhook: Mock, mock_pull_request: Mock + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock ) -> None: mock_github_webhook.ai_review_config["triggers"] = ["pr-opened"] - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-synchronized") + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-synchronized" + ) mock_github_webhook.logger.debug.assert_called_once() + mock_check_run_handler.set_check_in_progress.assert_not_called() @pytest.mark.asyncio - async def test_command_trigger_always_runs(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_command_trigger_always_runs( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: """When trigger is None (command-triggered), always run regardless of configured triggers.""" mock_response = Mock() - mock_response.status_code = 200 mock_response.raise_for_status = Mock() mock_response.json.return_value = {"status": "healthy"} review_response = Mock() - review_response.status_code = 200 review_response.raise_for_status = Mock() review_response.json.return_value = { "review_posted": True, @@ -77,12 +92,15 @@ async def test_command_trigger_always_runs(self, mock_github_webhook: Mock, mock mock_client_cls.return_value = mock_client with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger=None) + await call_ai_reviewer(mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger=None) mock_client.post.assert_called_once() + mock_check_run_handler.set_check_success.assert_called_once() @pytest.mark.asyncio - async def test_health_check_failure_posts_comment(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_health_check_failure_sets_check_failure( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: mock_client = AsyncMock() mock_client.__aenter__ = AsyncMock(return_value=mock_client) @@ -90,14 +108,18 @@ async def test_health_check_failure_posts_comment(self, mock_github_webhook: Moc mock_client.get = AsyncMock(side_effect=httpx.ConnectError("Connection refused")) mock_client_cls.return_value = mock_client - with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + await call_ai_reviewer(mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened") - mock_github_webhook.logger.warning.assert_called_once() - mock_pull_request.create_issue_comment.assert_called_once() + mock_check_run_handler.set_check_in_progress.assert_called_once() + mock_check_run_handler.set_check_failure.assert_called_once() + call_args = mock_check_run_handler.set_check_failure.call_args + assert call_args.kwargs["name"] == AI_REVIEW_STR + assert "not responding" in call_args.kwargs["output"]["summary"] @pytest.mark.asyncio - async def test_successful_review(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_successful_review( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: health_response = Mock() health_response.raise_for_status = Mock() @@ -118,12 +140,50 @@ async def test_successful_review(self, mock_github_webhook: Mock, mock_pull_requ mock_client_cls.return_value = mock_client with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" + ) - mock_github_webhook.logger.info.assert_called() + mock_check_run_handler.set_check_success.assert_called_once() + call_args = mock_check_run_handler.set_check_success.call_args + assert "1 comment(s) posted" in call_args.kwargs["output"]["summary"] @pytest.mark.asyncio - async def test_review_http_error(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_successful_review_no_issues( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: + health_response = Mock() + health_response.raise_for_status = Mock() + + review_response = Mock() + review_response.raise_for_status = Mock() + review_response.json.return_value = { + "review_posted": False, + "comments": [], + "summary": "No issues found", + } + + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + mock_client.get = AsyncMock(return_value=health_response) + mock_client.post = AsyncMock(return_value=review_response) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" + ) + + mock_check_run_handler.set_check_success.assert_called_once() + call_args = mock_check_run_handler.set_check_success.call_args + assert "No issues found" in call_args.kwargs["output"]["summary"] + + @pytest.mark.asyncio + async def test_review_http_error_sets_check_failure( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: health_response = Mock() health_response.raise_for_status = Mock() @@ -141,12 +201,18 @@ async def test_review_http_error(self, mock_github_webhook: Mock, mock_pull_requ mock_client_cls.return_value = mock_client with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" + ) - mock_github_webhook.logger.error.assert_called_once() + mock_check_run_handler.set_check_failure.assert_called_once() + call_args = mock_check_run_handler.set_check_failure.call_args + assert "failed" in call_args.kwargs["output"]["summary"].lower() @pytest.mark.asyncio - async def test_review_invalid_json(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_review_invalid_json_sets_check_failure( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: health_response = Mock() health_response.raise_for_status = Mock() @@ -163,24 +229,37 @@ async def test_review_invalid_json(self, mock_github_webhook: Mock, mock_pull_re mock_client_cls.return_value = mock_client with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" + ) - mock_github_webhook.logger.error.assert_called_once() + mock_check_run_handler.set_check_failure.assert_called_once() + call_args = mock_check_run_handler.set_check_failure.call_args + assert "Invalid JSON" in call_args.kwargs["output"]["summary"] @pytest.mark.asyncio - async def test_unexpected_exception(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_unexpected_exception_sets_check_failure( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: with patch("webhook_server.libs.ai_review.httpx.AsyncClient", side_effect=RuntimeError("boom")): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + await call_ai_reviewer(mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened") mock_github_webhook.logger.exception.assert_called_once() + mock_check_run_handler.set_check_failure.assert_called_once() @pytest.mark.asyncio - async def test_cancelled_error_reraised(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_cancelled_error_reraised( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: with patch("webhook_server.libs.ai_review.httpx.AsyncClient", side_effect=asyncio.CancelledError): with pytest.raises(asyncio.CancelledError): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" + ) @pytest.mark.asyncio - async def test_provider_config_conversion(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: + async def test_provider_config_conversion( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: """Verify YAML-format providers are converted to API format.""" mock_github_webhook.ai_review_config["providers"] = [ {"ai-provider": "claude", "ai-model": "sonnet"}, @@ -203,9 +282,10 @@ async def test_provider_config_conversion(self, mock_github_webhook: Mock, mock_ mock_client_cls.return_value = mock_client with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): - await call_ai_reviewer(mock_github_webhook, mock_pull_request, trigger="pr-opened") + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" + ) - # Check the payload sent to the service call_args = mock_client.post.call_args payload = call_args.kwargs.get("json") or call_args[1].get("json") assert payload["providers"] == [ diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 7fc8dff5..cadf5d1e 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -3318,6 +3318,7 @@ async def test_ai_review_triggered_on_opened( mock_ai_reviewer.assert_called_once_with( github_webhook=pull_request_handler.github_webhook, pull_request=mock_pull_request, + check_run_handler=pull_request_handler.check_run_handler, trigger="pr-opened", ) assert mock_create_task.call_count == 2 @@ -3381,6 +3382,7 @@ async def test_ai_review_triggered_on_synchronize_non_clean( mock_ai_reviewer.assert_called_once_with( github_webhook=pull_request_handler.github_webhook, pull_request=mock_pull_request, + check_run_handler=pull_request_handler.check_run_handler, trigger="pr-synchronized", ) assert mock_create_task.call_count == 2 diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 8c2911e8..aaba22c2 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -11,6 +11,7 @@ IN_PROGRESS_STR: str = "in_progress" QUEUED_STR: str = "queued" ADD_STR: str = "add" +AI_REVIEW_STR: str = "AI-Review" DELETE_STR: str = "delete" CAN_BE_MERGED_STR: str = "can-be-merged" BUILD_CONTAINER_STR: str = "build-container" From 5f05db8e8cd6c3a119c9bc8c824f7be47bdfeed4 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 13 Apr 2026 13:43:41 +0000 Subject: [PATCH 3/3] fix: address CodeRabbit review comments - Extract ai-review schema to $defs, use $ref in both locations - Remove unnecessary asyncio.to_thread for cached html_url property - Combine nested with statements (SIM117) --- webhook_server/config/schema.yaml | 201 +++++++++---------------- webhook_server/libs/ai_review.py | 16 +- webhook_server/tests/test_ai_review.py | 38 ++++- 3 files changed, 116 insertions(+), 139 deletions(-) diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index 52ae6a78..3b0ca19e 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -68,6 +68,73 @@ $defs: - ai-provider - ai-model additionalProperties: false + ai-review: + type: object + description: | + AI-powered code review for pull requests. + See https://github.com/myk-org/pr-ai-reviewer for server setup. + Each provider uses 3 specialized review agents (quality, guidelines, security). + Single provider: standard review. + Multiple providers: peer review (first = main, rest = peers). + They debate until consensus, then final comments are posted. + properties: + server-url: + type: string + format: uri + description: URL of the pr-ai-reviewer server (e.g., http://localhost:8001) + providers: + type: array + description: | + List of AI providers for code review. + Single provider: standard review. + Multiple providers: peer review (first = main reviewer, rest = peers). + items: + type: object + properties: + ai-provider: + type: string + enum: + - claude + - gemini + - cursor + description: AI CLI provider to use for review + ai-model: + type: string + description: AI model identifier + required: + - ai-provider + - ai-model + additionalProperties: false + minItems: 1 + max-rounds: + type: integer + minimum: 1 + maximum: 5 + default: 3 + description: Maximum peer review debate rounds before accepting main AI result + timeout-minutes: + type: integer + minimum: 1 + default: 30 + description: Timeout in minutes for each AI CLI call + triggers: + type: array + default: + - pr-opened + - pr-synchronized + items: + type: string + enum: + - pr-opened + - pr-synchronized + description: | + When to automatically run AI code review. Default: [pr-opened, pr-synchronized]. + - pr-opened: Run when a new PR is opened + - pr-synchronized: Run when new commits are pushed to a PR (skips clean rebases) + required: + - server-url + - providers + additionalProperties: false type: object properties: log-level: @@ -214,72 +281,7 @@ properties: ai-features: $ref: '#/$defs/ai-features' ai-review: - type: object - description: | - AI-powered code review for pull requests. - See https://github.com/myk-org/pr-ai-reviewer for server setup. - Each provider uses 3 specialized review agents (quality, guidelines, security). - Single provider: standard review. - Multiple providers: peer review (first = main, rest = peers). - They debate until consensus, then final comments are posted. - properties: - server-url: - type: string - format: uri - description: URL of the pr-ai-reviewer server (e.g., http://localhost:8001) - providers: - type: array - description: | - List of AI providers for code review. - Single provider: standard review. - Multiple providers: peer review (first = main reviewer, rest = peers). - items: - type: object - properties: - ai-provider: - type: string - enum: - - claude - - gemini - - cursor - description: AI CLI provider to use for review - ai-model: - type: string - description: AI model identifier - required: - - ai-provider - - ai-model - additionalProperties: false - minItems: 1 - max-rounds: - type: integer - minimum: 1 - maximum: 5 - default: 3 - description: Maximum peer review debate rounds before accepting main AI result - timeout-minutes: - type: integer - minimum: 1 - default: 30 - description: Timeout in minutes for each AI CLI call - triggers: - type: array - default: - - pr-opened - - pr-synchronized - items: - type: string - enum: - - pr-opened - - pr-synchronized - description: | - When to automatically run AI code review. Default: [pr-opened, pr-synchronized]. - - pr-opened: Run when a new PR is opened - - pr-synchronized: Run when new commits are pushed to a PR (skips clean rebases) - required: - - server-url - - providers - additionalProperties: false + $ref: '#/$defs/ai-review' labels: type: object description: Configure which labels are enabled and their colors @@ -645,72 +647,7 @@ properties: ai-features: $ref: '#/$defs/ai-features' ai-review: - type: object - description: | - AI-powered code review for pull requests. - See https://github.com/myk-org/pr-ai-reviewer for server setup. - Each provider uses 3 specialized review agents (quality, guidelines, security). - Single provider: standard review. - Multiple providers: peer review (first = main, rest = peers). - They debate until consensus, then final comments are posted. - properties: - server-url: - type: string - format: uri - description: URL of the pr-ai-reviewer server (e.g., http://localhost:8001) - providers: - type: array - description: | - List of AI providers for code review. - Single provider: standard review. - Multiple providers: peer review (first = main reviewer, rest = peers). - items: - type: object - properties: - ai-provider: - type: string - enum: - - claude - - gemini - - cursor - description: AI CLI provider to use for review - ai-model: - type: string - description: AI model identifier - required: - - ai-provider - - ai-model - additionalProperties: false - minItems: 1 - max-rounds: - type: integer - minimum: 1 - maximum: 5 - default: 3 - description: Maximum peer review debate rounds before accepting main AI result - timeout-minutes: - type: integer - minimum: 1 - default: 30 - description: Timeout in minutes for each AI CLI call - triggers: - type: array - default: - - pr-opened - - pr-synchronized - items: - type: string - enum: - - pr-opened - - pr-synchronized - description: | - When to automatically run AI code review. Default: [pr-opened, pr-synchronized]. - - pr-opened: Run when a new PR is opened - - pr-synchronized: Run when new commits are pushed to a PR (skips clean rebases) - required: - - server-url - - providers - additionalProperties: false + $ref: '#/$defs/ai-review' custom-check-runs: type: array description: | diff --git a/webhook_server/libs/ai_review.py b/webhook_server/libs/ai_review.py index fc22b0fe..4a7b1a7a 100644 --- a/webhook_server/libs/ai_review.py +++ b/webhook_server/libs/ai_review.py @@ -50,8 +50,10 @@ async def call_ai_reviewer( server_url: str = config["server-url"] log_prefix: str = github_webhook.log_prefix + check_in_progress = False try: await check_run_handler.set_check_in_progress(name=AI_REVIEW_STR) + check_in_progress = True async with httpx.AsyncClient(base_url=server_url) as client: # Health check @@ -72,7 +74,7 @@ async def call_ai_reviewer( return # Build review payload - pr_url: str = await asyncio.to_thread(lambda: pull_request.html_url) + pr_url: str = pull_request.html_url # Convert provider configs from YAML format to API format providers_config: list[dict[str, str]] = config.get("providers", []) @@ -122,7 +124,9 @@ async def call_ai_reviewer( }, ) except httpx.HTTPError as e: - err_detail = f": {e.response.text}" if isinstance(e, httpx.HTTPStatusError) else "" + err_detail = "" + if isinstance(e, httpx.HTTPStatusError): + err_detail = f": {e.response.text[:2000]}" error_msg = f"AI Reviewer request failed{err_detail}" github_webhook.logger.error(f"{log_prefix} {error_msg}") await check_run_handler.set_check_failure( @@ -136,6 +140,14 @@ async def call_ai_reviewer( output={"title": "AI Review Failed", "summary": "Invalid JSON response from AI Reviewer"}, ) except asyncio.CancelledError: + if check_in_progress: + try: + await check_run_handler.set_check_failure( + name=AI_REVIEW_STR, + output={"title": "AI Review Cancelled", "summary": "Review was cancelled"}, + ) + except Exception: + github_webhook.logger.exception(f"{log_prefix} Failed to set check run failure on cancellation") raise except Exception: github_webhook.logger.exception(f"{log_prefix} AI Reviewer call failed unexpectedly") diff --git a/webhook_server/tests/test_ai_review.py b/webhook_server/tests/test_ai_review.py index 6021eb4f..bd7086ea 100644 --- a/webhook_server/tests/test_ai_review.py +++ b/webhook_server/tests/test_ai_review.py @@ -95,6 +95,7 @@ async def test_command_trigger_always_runs( await call_ai_reviewer(mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger=None) mock_client.post.assert_called_once() + mock_check_run_handler.set_check_in_progress.assert_called_once() mock_check_run_handler.set_check_success.assert_called_once() @pytest.mark.asyncio @@ -250,11 +251,38 @@ async def test_unexpected_exception_sets_check_failure( async def test_cancelled_error_reraised( self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock ) -> None: - with patch("webhook_server.libs.ai_review.httpx.AsyncClient", side_effect=asyncio.CancelledError): - with pytest.raises(asyncio.CancelledError): - await call_ai_reviewer( - mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" - ) + with ( + patch("webhook_server.libs.ai_review.httpx.AsyncClient", side_effect=asyncio.CancelledError), + pytest.raises(asyncio.CancelledError), + ): + await call_ai_reviewer(mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened") + + @pytest.mark.asyncio + async def test_cancelled_error_sets_check_failure_when_in_progress( + self, mock_github_webhook: Mock, mock_pull_request: Mock, mock_check_run_handler: AsyncMock + ) -> None: + """CancelledError after check is in_progress should mark it as failed before re-raising.""" + with patch("webhook_server.libs.ai_review.httpx.AsyncClient") as mock_client_cls: + mock_client = AsyncMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + # Health check succeeds, but review call raises CancelledError + mock_client.get = AsyncMock(return_value=Mock(raise_for_status=Mock())) + mock_client.post = AsyncMock(side_effect=asyncio.CancelledError) + mock_client_cls.return_value = mock_client + + with patch("asyncio.to_thread", side_effect=lambda f, *a, **kw: f(*a, **kw) if callable(f) else f): + with pytest.raises(asyncio.CancelledError): + await call_ai_reviewer( + mock_github_webhook, mock_pull_request, mock_check_run_handler, trigger="pr-opened" + ) + + # Verify check was marked in_progress then failed on cancellation + mock_check_run_handler.set_check_in_progress.assert_called_once() + mock_check_run_handler.set_check_failure.assert_called_once() + call_args = mock_check_run_handler.set_check_failure.call_args + assert call_args.kwargs["output"]["title"] == "AI Review Cancelled" + assert call_args.kwargs["output"]["summary"] == "Review was cancelled" @pytest.mark.asyncio async def test_provider_config_conversion(