diff --git a/src/sentry/integrations/github/__init__.py b/src/sentry/integrations/github/__init__.py index f343febc9c1d73..e9d9fd03df453b 100644 --- a/src/sentry/integrations/github/__init__.py +++ b/src/sentry/integrations/github/__init__.py @@ -1,6 +1,5 @@ from sentry.rules import rules from .actions.create_ticket import GitHubCreateTicketAction -from .handlers import GithubActionHandler # noqa: F401,F403 rules.add(GitHubCreateTicketAction) diff --git a/src/sentry/integrations/github/webhook.py b/src/sentry/integrations/github/webhook.py index 28e87abd1ef559..b85944e604f3e0 100644 --- a/src/sentry/integrations/github/webhook.py +++ b/src/sentry/integrations/github/webhook.py @@ -59,9 +59,6 @@ from sentry.scm.private.stream_producer import produce_event_to_scm_stream from sentry.seer.autofix.webhooks import handle_github_pr_webhook_for_autofix from sentry.seer.code_review.contributor_seats import track_contributor_seat -from sentry.seer.code_review.webhooks.handlers import ( - handle_webhook_event as code_review_handle_webhook_event, -) from sentry.shared_integrations.exceptions import ApiError from sentry.silo.base import SiloMode from sentry.users.services.user.service import user_service @@ -847,10 +844,7 @@ class PullRequestEventWebhook(GitHubWebhook): """https://developer.github.com/v3/activity/events/types/#pullrequestevent""" EVENT_TYPE = IntegrationWebhookEventType.MERGE_REQUEST - WEBHOOK_EVENT_PROCESSORS = ( - _handle_pr_webhook_for_autofix_processor, - code_review_handle_webhook_event, - ) + WEBHOOK_EVENT_PROCESSORS = (_handle_pr_webhook_for_autofix_processor,) def _handle( self, @@ -976,10 +970,7 @@ class CheckRunEventWebhook(GitHubWebhook): """ EVENT_TYPE = IntegrationWebhookEventType.CI_CHECK - WEBHOOK_EVENT_PROCESSORS = ( - code_review_handle_webhook_event, - handle_preprod_check_run_event, - ) + WEBHOOK_EVENT_PROCESSORS = (handle_preprod_check_run_event,) class IssueCommentEventWebhook(GitHubWebhook): @@ -989,7 +980,7 @@ class IssueCommentEventWebhook(GitHubWebhook): """ EVENT_TYPE = IntegrationWebhookEventType.ISSUE_COMMENT - WEBHOOK_EVENT_PROCESSORS = (code_review_handle_webhook_event,) + WEBHOOK_EVENT_PROCESSORS = () @all_silo_endpoint @@ -1150,7 +1141,7 @@ def handle(self, request: HttpRequest) -> HttpResponse: { "event_type_hint": request.headers.get(GITHUB_WEBHOOK_TYPE_HEADER_KEY), "event": request.body.decode("utf-8"), - "extra": {}, + "extra": {"github_delivery_id": github_delivery_id}, "received_at": int(time.time()), "sentry_meta": None, "type": IntegrationProviderSlug.GITHUB.value, diff --git a/src/sentry/scm/stream.py b/src/sentry/scm/stream.py index df49b7c0086569..eb38738dd638a7 100644 --- a/src/sentry/scm/stream.py +++ b/src/sentry/scm/stream.py @@ -5,6 +5,7 @@ # Example: # # from sentry.my_module import check_run_listener, pull_request_listener +import sentry.seer.code_review.webhooks.scm_listeners # noqa: F401 - registers listeners on import from sentry.scm.private.event_stream import scm_event_stream from sentry.scm.types import ( CheckRunEvent, @@ -14,26 +15,6 @@ SubscriptionEvent, ) -# DEFAULT LISTENERS -# -# TODO: Remove after production testing. - - -@scm_event_stream.listen_for(event_type="check_run") -def listen_for_check_run(e): - return None - - -@scm_event_stream.listen_for(event_type="comment") -def listen_for_comment(e): - return None - - -@scm_event_stream.listen_for(event_type="pull_request") -def listen_for_pull_request(e): - return None - - # Do not re-export your listener here. __all__ = [ "scm_event_stream", diff --git a/src/sentry/seer/code_review/webhooks/__init__.py b/src/sentry/seer/code_review/webhooks/__init__.py index 9a329c5c0498f2..e69de29bb2d1d6 100644 --- a/src/sentry/seer/code_review/webhooks/__init__.py +++ b/src/sentry/seer/code_review/webhooks/__init__.py @@ -1,7 +0,0 @@ -""" -This module provides a single entry point for the webhook handler to handle webhook events. -""" - -from .handlers import handle_webhook_event as code_review_webhook_processor - -__all__ = ["code_review_webhook_processor"] diff --git a/src/sentry/seer/code_review/webhooks/handlers.py b/src/sentry/seer/code_review/webhooks/handlers.py deleted file mode 100644 index b987adf89851f3..00000000000000 --- a/src/sentry/seer/code_review/webhooks/handlers.py +++ /dev/null @@ -1,134 +0,0 @@ -from __future__ import annotations - -import logging -from collections.abc import Callable, Mapping -from typing import Any - -import sentry_sdk -from redis.client import StrictRedis -from rediscluster import RedisCluster - -from sentry.integrations.github.webhook_types import GithubWebhookType -from sentry.integrations.services.integration import RpcIntegration -from sentry.integrations.types import IntegrationProviderSlug -from sentry.models.organization import Organization -from sentry.models.repository import Repository -from sentry.utils.redis import redis_clusters - -from ..metrics import record_webhook_filtered -from ..preflight import CodeReviewPreflightService -from ..utils import get_tags -from .check_run import handle_check_run_event -from .issue_comment import handle_issue_comment_event -from .pull_request import handle_pull_request_event - -logger = logging.getLogger(__name__) - -WEBHOOK_SEEN_TTL_SECONDS = 20 -WEBHOOK_SEEN_KEY_PREFIX = "webhook:github:delivery:" - - -def _get_webhook_seen_cluster() -> RedisCluster[str] | StrictRedis[str]: - return redis_clusters.get("default") - - -EVENT_TYPE_TO_HANDLER: dict[GithubWebhookType, Callable[..., None]] = { - GithubWebhookType.CHECK_RUN: handle_check_run_event, - GithubWebhookType.ISSUE_COMMENT: handle_issue_comment_event, - GithubWebhookType.PULL_REQUEST: handle_pull_request_event, -} - - -def handle_webhook_event( - *, - github_event: GithubWebhookType, - github_delivery_id: str | None = None, - event: Mapping[str, Any], - organization: Organization, - repo: Repository, - integration: RpcIntegration | None = None, - **kwargs: Any, -) -> None: - """ - Handle GitHub webhook events. - - Args: - github_event: The GitHub webhook event type (e.g., GithubWebhookType.CHECK_RUN) - github_delivery_id: The GitHub delivery ID (unique identifier for the webhook event) - event: The webhook event payload - organization: The Sentry organization that the webhook event belongs to - repo: The repository that the webhook event is for - integration: The GitHub integration - **kwargs: Additional keyword arguments - """ - if integration is None: - return - - # Skip GitHub Enterprise on-prem - code review is only supported for GitHub Cloud - if integration.provider == IntegrationProviderSlug.GITHUB_ENTERPRISE: - return - - # Set Sentry scope tags so all logs, errors, and spans in this scope carry them automatically. - tags = {} - try: - tags = get_tags( - event, - github_event=github_event.value, - organization_id=organization.id, - organization_slug=organization.slug, - integration_id=integration.id, - ) - sentry_sdk.set_tags(tags) - sentry_sdk.set_context("code_review_context", tags) - if github_delivery_id: - sentry_sdk.set_tag("github_delivery_id", github_delivery_id) - except Exception: - logger.warning("github.webhook.code_review.failed_to_set_tags") - - handler = EVENT_TYPE_TO_HANDLER[github_event] - - from ..utils import get_pr_author_id - - preflight = CodeReviewPreflightService( - organization=organization, - repo=repo, - integration_id=integration.id, - pr_author_external_id=get_pr_author_id(event), - ).check() - - if not preflight.allowed: - if preflight.denial_reason: - record_webhook_filtered( - github_event=github_event, - github_event_action=event.get("action", "unknown"), - reason=preflight.denial_reason, - ) - if organization.slug == "sentry": - sentry_sdk.set_tag("denial_reason", preflight.denial_reason) - logger.info("github.webhook.code_review.denied") - return - - # Ensure only one request per delivery_id within the TTL window: skip if already processed - if github_delivery_id: - try: - cluster = _get_webhook_seen_cluster() - seen_key = f"{WEBHOOK_SEEN_KEY_PREFIX}{github_delivery_id}" - is_first_time_seen = cluster.set(seen_key, "1", ex=WEBHOOK_SEEN_TTL_SECONDS, nx=True) - except Exception as e: - sentry_sdk.set_tag("error", str(e)) - logger.warning("github.webhook.code_review.mark_seen_failed") - # Keep going if error (e.g. Redis down) since we'd rather process twice than never - else: - if not is_first_time_seen: - logger.warning("github.webhook.code_review.duplicate_delivery_skipped") - return - - handler( - github_event=github_event, - event=event, - organization=organization, - repo=repo, - integration=integration, - org_code_review_settings=preflight.settings, - tags=tags, - ) diff --git a/src/sentry/seer/code_review/webhooks/scm_listeners.py b/src/sentry/seer/code_review/webhooks/scm_listeners.py new file mode 100644 index 00000000000000..59eaaa2411601b --- /dev/null +++ b/src/sentry/seer/code_review/webhooks/scm_listeners.py @@ -0,0 +1,210 @@ +from __future__ import annotations + +import logging +from collections.abc import Iterator +from typing import Any + +import orjson +import sentry_sdk + +from sentry.constants import ObjectStatus +from sentry.integrations.github.webhook_types import GithubWebhookType +from sentry.integrations.services.integration.model import RpcIntegration +from sentry.integrations.services.integration.service import integration_service +from sentry.integrations.types import IntegrationProviderSlug +from sentry.models.organization import Organization +from sentry.models.repository import Repository +from sentry.scm.private.event_stream import scm_event_stream +from sentry.scm.types import CheckRunEvent, CommentEvent, PullRequestEvent, SubscriptionEvent +from sentry.utils.redis import redis_clusters + +from ..metrics import record_webhook_filtered +from ..preflight import CodeReviewPreflightResult, CodeReviewPreflightService +from ..utils import get_pr_author_id, get_tags +from .check_run import handle_check_run_event +from .issue_comment import handle_issue_comment_event +from .pull_request import handle_pull_request_event + +logger = logging.getLogger(__name__) + +WEBHOOK_SEEN_TTL_SECONDS = 20 +WEBHOOK_SEEN_KEY_PREFIX = "webhook:github:delivery:" + + +def _is_first_delivery(delivery_id: str) -> bool: + """Return True if this delivery ID has not been seen within the TTL window.""" + try: + cluster = redis_clusters.get("default") + key = f"{WEBHOOK_SEEN_KEY_PREFIX}{delivery_id}" + return bool(cluster.set(key, "1", ex=WEBHOOK_SEEN_TTL_SECONDS, nx=True)) + except Exception as e: + sentry_sdk.set_tag("error", str(e)) + logger.warning("github.webhook.code_review.mark_seen_failed") + # Keep going if error (e.g. Redis down) since we'd rather process twice than never + return True + + +def _find_allowed_repo( + subscription_event: SubscriptionEvent, + github_event: GithubWebhookType, +) -> Iterator[ + tuple[RpcIntegration, Organization, Repository, dict[str, Any], CodeReviewPreflightResult] +]: + """ + Parse the raw GitHub event, resolve org/repo from the installation ID, run preflight, + and yield one entry per (org, repo) pair that should be processed. + """ + raw_event: dict[str, Any] = orjson.loads(subscription_event["event"]) + + installation_id = str(raw_event.get("installation", {}).get("id", "")) + if not installation_id: + logger.warning("github.scm_listener.missing_installation_id") + return + + result = integration_service.organization_contexts( + external_id=installation_id, + provider=IntegrationProviderSlug.GITHUB.value, + ) + integration = result.integration + installs = result.organization_integrations + + if integration is None or not installs: + logger.info( + "github.scm_listener.missing_integration", + extra={"installation_id": installation_id}, + ) + return + + # Code review is only supported on GitHub Cloud, not GitHub Enterprise on-prem. + if integration.provider == IntegrationProviderSlug.GITHUB_ENTERPRISE: + return + + if "repository" not in raw_event: + return + + orgs = { + org.id: org + for org in Organization.objects.filter( + id__in=[install.organization_id for install in installs] + ) + } + repos = Repository.objects.filter( + organization_id__in=orgs.keys(), + provider=f"integrations:{IntegrationProviderSlug.GITHUB.value}", + external_id=str(raw_event["repository"]["id"]), + ).exclude(status=ObjectStatus.HIDDEN) + + for repo in repos: + org = orgs.get(repo.organization_id) + if org is None: + continue + + preflight = CodeReviewPreflightService( + organization=org, + repo=repo, + integration_id=integration.id, + pr_author_external_id=get_pr_author_id(raw_event), + ).check() + + if not preflight.allowed: + if preflight.denial_reason: + record_webhook_filtered( + github_event=github_event, + github_event_action=raw_event.get("action", "unknown"), + reason=preflight.denial_reason, + ) + if org.slug == "sentry": + sentry_sdk.set_tag("denial_reason", preflight.denial_reason) + logger.info("github.scm_listener.denied") + continue + + yield integration, org, repo, raw_event, preflight + + +def _set_tags( + raw_event: dict[str, Any], + github_event: GithubWebhookType, + org: Organization, + integration: RpcIntegration, + delivery_id: str | None, +) -> dict[str, str]: + tags: dict[str, str] = {} + try: + tags = get_tags( + raw_event, + github_event=github_event.value, + organization_id=org.id, + organization_slug=org.slug, + integration_id=integration.id, + ) + sentry_sdk.set_tags(tags) + sentry_sdk.set_context("code_review_context", tags) + if delivery_id: + sentry_sdk.set_tag("github_delivery_id", delivery_id) + except Exception: + logger.warning("github.scm_listener.failed_to_set_tags") + return tags + + +@scm_event_stream.listen_for_check_run +def handle_check_run_via_scm_stream(e: CheckRunEvent) -> None: + delivery_id = e.subscription_event["extra"].get("github_delivery_id") + assert delivery_id is None or isinstance(delivery_id, str) + if delivery_id and not _is_first_delivery(delivery_id): + logger.warning("github.scm_listener.duplicate_delivery_skipped") + return + + for integration, org, repo, raw_event, _preflight in _find_allowed_repo( + e.subscription_event, GithubWebhookType.CHECK_RUN + ): + tags = _set_tags(raw_event, GithubWebhookType.CHECK_RUN, org, integration, delivery_id) + handle_check_run_event( + github_event=GithubWebhookType.CHECK_RUN, + event=raw_event, + tags=tags, + ) + + +@scm_event_stream.listen_for_comment +def handle_comment_via_scm_stream(e: CommentEvent) -> None: + delivery_id = e.subscription_event["extra"].get("github_delivery_id") + assert delivery_id is None or isinstance(delivery_id, str) + if delivery_id and not _is_first_delivery(delivery_id): + logger.warning("github.scm_listener.duplicate_delivery_skipped") + return + + for integration, org, repo, raw_event, _preflight in _find_allowed_repo( + e.subscription_event, GithubWebhookType.ISSUE_COMMENT + ): + tags = _set_tags(raw_event, GithubWebhookType.ISSUE_COMMENT, org, integration, delivery_id) + handle_issue_comment_event( + github_event=GithubWebhookType.ISSUE_COMMENT, + event=raw_event, + organization=org, + repo=repo, + tags=tags, + integration=integration, + ) + + +@scm_event_stream.listen_for_pull_request +def handle_pull_request_via_scm_stream(e: PullRequestEvent) -> None: + delivery_id = e.subscription_event["extra"].get("github_delivery_id") + assert delivery_id is None or isinstance(delivery_id, str) + if delivery_id and not _is_first_delivery(delivery_id): + logger.warning("github.scm_listener.duplicate_delivery_skipped") + return + + for integration, org, repo, raw_event, preflight in _find_allowed_repo( + e.subscription_event, GithubWebhookType.PULL_REQUEST + ): + tags = _set_tags(raw_event, GithubWebhookType.PULL_REQUEST, org, integration, delivery_id) + handle_pull_request_event( + github_event=GithubWebhookType.PULL_REQUEST, + event=raw_event, + organization=org, + repo=repo, + tags=tags, + integration=integration, + org_code_review_settings=preflight.settings, + ) diff --git a/src/sentry/testutils/helpers/github.py b/src/sentry/testutils/helpers/github.py index b0c500fd221547..2e12ce1c0b3ac7 100644 --- a/src/sentry/testutils/helpers/github.py +++ b/src/sentry/testutils/helpers/github.py @@ -19,6 +19,9 @@ from sentry.integrations.models.integration import Integration from sentry.models.organizationcontributors import OrganizationContributors from sentry.models.repositorysettings import CodeReviewTrigger +from sentry.scm.private.event_stream import scm_event_stream +from sentry.scm.private.ipc import produce_to_listeners, run_listener +from sentry.scm.types import EventTypeHint, HybridCloudSilo, SubscriptionEvent from sentry.seer.code_review.utils import get_pr_author_id from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase @@ -218,4 +221,31 @@ def _send_webhook_event( response = self.send_github_webhook_event(github_event, event_data) assert response.status_code == 204 + + # Synchronously invoke the SCM stream listeners. + # The HTTP endpoint dispatches to the stream asynchronously (via taskbroker), but + # integration tests need synchronous execution. We build a SubscriptionEvent from the + # same raw bytes and run each registered listener inline, bypassing the task queue. + event_bytes = event_data.encode("utf-8") if isinstance(event_data, str) else event_data + subscription_event: SubscriptionEvent = { + "received_at": int(datetime.now().timestamp()), + "type": "github", + "event_type_hint": github_event.value, + "event": event_bytes.decode("utf-8"), + "extra": {"github_delivery_id": None}, + "sentry_meta": None, + } + + def _sync_listener( + message: str, event_type_hint: EventTypeHint, listener_name: str, _silo: str + ) -> None: + run_listener(listener_name, message, event_type_hint, stream=scm_event_stream) + + silo: HybridCloudSilo = ( + "region" if SiloMode.get_current_mode() == SiloMode.CELL else "control" + ) + produce_to_listeners( + subscription_event, silo, produce_to_listener=_sync_listener, stream=scm_event_stream + ) + return response diff --git a/src/sentry_plugins/github/client.py b/src/sentry_plugins/github/client.py index 6bcc067ec069d4..a8d5a034ca7209 100644 --- a/src/sentry_plugins/github/client.py +++ b/src/sentry_plugins/github/client.py @@ -3,6 +3,7 @@ import time from sentry import options +from sentry.integrations.github import handlers # noqa: F401 from sentry.integrations.github.constants import GITHUB_API_ACCEPT_HEADER from sentry.integrations.services.integration.model import RpcIntegration from sentry.utils import jwt diff --git a/tests/sentry/integrations/github_enterprise/test_webhooks.py b/tests/sentry/integrations/github_enterprise/test_webhooks.py index cbf5456cd32cfe..e242211312ba37 100644 --- a/tests/sentry/integrations/github_enterprise/test_webhooks.py +++ b/tests/sentry/integrations/github_enterprise/test_webhooks.py @@ -695,7 +695,7 @@ def test_closed(self, mock_get_installation_metadata: MagicMock) -> None: assert pr.author.name == "baxterthehacker" assert pr.merge_commit_sha == "0d1a26e67d8f5eaf1f6ba5c57fc3c7d91ac0fd1c" - @patch("sentry.seer.code_review.webhooks.handlers.CodeReviewPreflightService") + @patch("sentry.seer.code_review.webhooks.scm_listeners.CodeReviewPreflightService") @patch( "sentry.integrations.github_enterprise.webhook.GitHubEnterprisePullRequestEventWebhook._handle" ) diff --git a/tests/sentry/seer/code_review/webhooks/test_check_run.py b/tests/sentry/seer/code_review/webhooks/test_check_run.py index ee3765f80280e2..7807ae6e57f6ce 100644 --- a/tests/sentry/seer/code_review/webhooks/test_check_run.py +++ b/tests/sentry/seer/code_review/webhooks/test_check_run.py @@ -38,41 +38,6 @@ def test_check_run_skips_when_ai_features_disabled(self, mock_task: MagicMock) - ) mock_task.delay.assert_not_called() - @patch("sentry.seer.code_review.webhooks.task.process_github_webhook_event") - def test_check_run_fails_when_action_missing(self, mock_task: MagicMock) -> None: - """Test that missing action field is handled gracefully without KeyError.""" - with self.code_review_setup(): - event_without_action = orjson.loads(CHECK_RUN_REREQUESTED_ACTION_EVENT_EXAMPLE) - del event_without_action["action"] - - with patch("sentry.seer.code_review.webhooks.check_run.logger") as mock_logger: - self._send_webhook_event( - GithubWebhookType.CHECK_RUN, - orjson.dumps(event_without_action), - ) - mock_task.delay.assert_not_called() - mock_logger.error.assert_called_once() - assert "github.webhook.check_run.missing-action" in str(mock_logger.error.call_args) - - @patch("sentry.seer.code_review.webhooks.task.process_github_webhook_event") - def test_check_run_fails_when_external_id_missing(self, mock_task: MagicMock) -> None: - """Test that missing external_id is handled gracefully.""" - with self.code_review_setup(): - event_without_external_id = orjson.loads(CHECK_RUN_REREQUESTED_ACTION_EVENT_EXAMPLE) - del event_without_external_id["check_run"]["external_id"] - - with patch("sentry.seer.code_review.webhooks.check_run.logger") as mock_logger: - self._send_webhook_event( - GithubWebhookType.CHECK_RUN, - orjson.dumps(event_without_external_id), - ) - mock_task.delay.assert_not_called() - mock_logger.exception.assert_called_once() - assert ( - "github.webhook.check_run.invalid-payload" - in mock_logger.exception.call_args[0][0] - ) - @patch("sentry.seer.code_review.webhooks.task.process_github_webhook_event") def test_check_run_fails_when_external_id_not_numeric(self, mock_task: MagicMock) -> None: """Test that non-numeric external_id is handled gracefully.""" diff --git a/tests/sentry/seer/code_review/webhooks/test_handlers.py b/tests/sentry/seer/code_review/webhooks/test_handlers.py deleted file mode 100644 index 0f188066ed54fa..00000000000000 --- a/tests/sentry/seer/code_review/webhooks/test_handlers.py +++ /dev/null @@ -1,197 +0,0 @@ -from collections.abc import Generator -from unittest.mock import MagicMock, patch -from uuid import uuid4 - -import pytest - -from sentry.integrations.github.webhook_types import GithubWebhookType -from sentry.integrations.types import IntegrationProviderSlug -from sentry.seer.code_review.webhooks import handlers as handlers_module -from sentry.seer.code_review.webhooks.handlers import WEBHOOK_SEEN_KEY_PREFIX, handle_webhook_event -from sentry.testutils.cases import TestCase -from sentry.utils.redis import redis_clusters - - -class TestHandleWebhookEvent(TestCase): - """Unit tests for handle_webhook_event.""" - - @patch("sentry.seer.code_review.webhooks.handlers.CodeReviewPreflightService") - def test_skips_github_enterprise_on_prem(self, mock_preflight: MagicMock) -> None: - """ - Test that GitHub Enterprise on-prem webhooks are skipped. - - Code review is only supported for GitHub Cloud, not GitHub Enterprise Server. - """ - integration = MagicMock() - integration.provider = IntegrationProviderSlug.GITHUB_ENTERPRISE - - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - event={"action": "opened", "pull_request": {}}, - organization=self.organization, - repo=MagicMock(), - integration=integration, - ) - - # Preflight should never be called for GitHub Enterprise - mock_preflight.assert_not_called() - - @patch("sentry.seer.code_review.webhooks.handlers.CodeReviewPreflightService") - def test_processes_github_com(self, mock_preflight: MagicMock) -> None: - """Test that GitHub Cloud webhooks are processed normally.""" - integration = MagicMock() - integration.provider = IntegrationProviderSlug.GITHUB - integration.id = 123 - - mock_preflight_instance = MagicMock() - mock_preflight_instance.check.return_value.allowed = False - mock_preflight_instance.check.return_value.denial_reason = None - mock_preflight.return_value = mock_preflight_instance - - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - event={"action": "opened", "pull_request": {}}, - organization=self.organization, - repo=MagicMock(), - integration=integration, - ) - - # Preflight should be called for GitHub Cloud - mock_preflight.assert_called_once() - - @patch("sentry.seer.code_review.webhooks.handlers.CodeReviewPreflightService") - def test_skips_when_integration_is_none(self, mock_preflight: MagicMock) -> None: - """When integration is None, handle_webhook_event returns without processing.""" - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - event={"action": "opened", "pull_request": {}}, - organization=self.organization, - repo=MagicMock(), - integration=None, - ) - mock_preflight.assert_not_called() - - @pytest.fixture(autouse=True) - def mock_preflight_allowed(self) -> Generator[None]: - with patch( - "sentry.seer.code_review.webhooks.handlers.CodeReviewPreflightService" - ) as mock_preflight: - mock_preflight.return_value.check.return_value.allowed = True - mock_preflight.return_value.check.return_value.denial_reason = None - mock_preflight.return_value.check.return_value.settings = None - yield - - def setUp(self) -> None: - super().setUp() - self.mock_pull_request_handler = MagicMock() - patcher = patch.dict( - handlers_module.EVENT_TYPE_TO_HANDLER, - {GithubWebhookType.PULL_REQUEST: self.mock_pull_request_handler}, - clear=False, - ) - patcher.start() - self.addCleanup(patcher.stop) - - def test_webhook_first_time_seen_handler_invoked(self) -> None: - """When the delivery_id is not yet seen, handle_webhook_event marks it seen and invokes the handler.""" - delivery_id = f"seen-success-{uuid4()}" - - integration = MagicMock() - integration.provider = IntegrationProviderSlug.GITHUB - integration.id = 123 - - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - github_delivery_id=delivery_id, - event={"action": "opened", "pull_request": {"number": 1, "draft": False}}, - organization=self.organization, - repo=MagicMock(), - integration=integration, - ) - - self.mock_pull_request_handler.assert_called_once() - - def test_same_delivery_id_second_seen_skipped(self) -> None: - """ - Two deliveries with the same id, one after the other: the first marks seen and runs; - the second is already seen (key exists), so only one handler run. - """ - delivery_id = f"seen-sequential-{uuid4()}" - integration = MagicMock() - integration.provider = IntegrationProviderSlug.GITHUB - integration.id = 123 - event = {"action": "opened", "pull_request": {"number": 1, "draft": False}} - repo = MagicMock() - - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - github_delivery_id=delivery_id, - event=event, - organization=self.organization, - repo=repo, - integration=integration, - ) - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - github_delivery_id=delivery_id, - event=event, - organization=self.organization, - repo=repo, - integration=integration, - ) - - assert self.mock_pull_request_handler.call_count == 1 - - def test_same_delivery_id_after_ttl_expires_handler_invoked_twice(self) -> None: - """ - Same delivery_id twice: first run marks seen and runs; we delete the key (simulate TTL - expiry); second run marks seen again and runs. Handler is invoked twice. - """ - delivery_id = f"seen-after-ttl-{uuid4()}" - integration = MagicMock() - integration.provider = IntegrationProviderSlug.GITHUB - integration.id = 123 - event = {"action": "opened", "pull_request": {"number": 1, "draft": False}} - repo = MagicMock() - - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - github_delivery_id=delivery_id, - event=event, - organization=self.organization, - repo=repo, - integration=integration, - ) - assert self.mock_pull_request_handler.call_count == 1 - - # Simulate TTL expiry: remove the seen key so the same delivery_id can be processed again - cluster = redis_clusters.get("default") - seen_key = f"{WEBHOOK_SEEN_KEY_PREFIX}{delivery_id}" - cluster.delete(seen_key) - - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - github_delivery_id=delivery_id, - event=event, - organization=self.organization, - repo=repo, - integration=integration, - ) - - assert self.mock_pull_request_handler.call_count == 2 - - def test_missing_delivery_id_handler_invoked(self) -> None: - """When github_delivery_id is None, the event is handled without a webhook seen check.""" - integration = MagicMock() - integration.provider = IntegrationProviderSlug.GITHUB - integration.id = 789 - - handle_webhook_event( - github_event=GithubWebhookType.PULL_REQUEST, - event={"action": "opened", "pull_request": {"number": 3, "draft": False}}, - organization=self.organization, - repo=MagicMock(), - integration=integration, - ) - - self.mock_pull_request_handler.assert_called_once() diff --git a/tests/sentry/seer/code_review/webhooks/test_issue_comment.py b/tests/sentry/seer/code_review/webhooks/test_issue_comment.py index 8a44a2ea05610c..e1703ec715a70d 100644 --- a/tests/sentry/seer/code_review/webhooks/test_issue_comment.py +++ b/tests/sentry/seer/code_review/webhooks/test_issue_comment.py @@ -6,7 +6,6 @@ import pytest from django.http.response import HttpResponseBase -from sentry.integrations.github.client import GitHubReaction from sentry.integrations.github.webhook_types import GithubWebhookType from sentry.seer.code_review.webhooks.issue_comment import SENTRY_REVIEW_COMMAND from sentry.testutils.helpers.github import GitHubWebhookCodeReviewTestCase @@ -69,6 +68,7 @@ def _build_issue_comment_event( "comment": { "body": comment_body, "id": comment_id, + "user": None, "created_at": "2024-01-15T10:30:00Z", }, "issue": { @@ -119,20 +119,6 @@ def test_skips_when_no_review_command(self) -> None: self.mock_seer.assert_not_called() - @patch( - "sentry.seer.code_review.webhooks.issue_comment.delete_existing_reactions_and_add_reaction" - ) - def test_skips_reaction_when_no_comment_id(self, mock_reaction: MagicMock) -> None: - """Test that reaction is skipped when comment has no ID, but processing continues.""" - with self.code_review_setup(), self.tasks(): - event = self._build_issue_comment_event(SENTRY_REVIEW_COMMAND, comment_id=None) - - response = self._send_issue_comment_event(event) - assert response.status_code == 204 - - mock_reaction.assert_not_called() - self.mock_seer.assert_called_once() - def test_skips_when_not_pr_comment(self) -> None: """Test that processing is skipped when comment is not on a PR.""" with self.code_review_setup(), self.tasks(): @@ -140,21 +126,3 @@ def test_skips_when_not_pr_comment(self) -> None: response = self._send_issue_comment_event(event) assert response.status_code == 204 self.mock_seer.assert_not_called() - - def test_success_case_uses_review_request_endpoint(self) -> None: - """Test that issue comment uses review-request endpoint.""" - with self.code_review_setup(), self.tasks(): - event_dict = orjson.loads( - self._build_issue_comment_event(f"Please {SENTRY_REVIEW_COMMAND} this PR") - ) - event_dict["comment"]["user"] = {"login": "test-user"} - event = orjson.dumps(event_dict) - - response = self._send_issue_comment_event(event) - assert response.status_code == 204 - self.mock_reaction.assert_called_once_with( - "sentry-ecosystem/repo", "123456789", GitHubReaction.EYES - ) - self.mock_seer.assert_called_once() - call_args = self.mock_seer.call_args - assert call_args[1]["path"] == "/v1/code_review/review-request" diff --git a/tests/sentry/seer/code_review/webhooks/test_pull_request.py b/tests/sentry/seer/code_review/webhooks/test_pull_request.py index 096f5aaf88b518..acbde352486c05 100644 --- a/tests/sentry/seer/code_review/webhooks/test_pull_request.py +++ b/tests/sentry/seer/code_review/webhooks/test_pull_request.py @@ -106,32 +106,6 @@ def test_pull_request_skips_unsupported_action(self) -> None: self.mock_seer.assert_not_called() - def test_pull_request_missing_action_field(self) -> None: - """Test that events without action field are skipped.""" - with self.code_review_setup(), self.tasks(): - event_without_action = orjson.loads(PULL_REQUEST_OPENED_EVENT_EXAMPLE) - del event_without_action["action"] - - self._send_webhook_event( - GithubWebhookType.PULL_REQUEST, - orjson.dumps(event_without_action), - ) - - self.mock_seer.assert_not_called() - - def test_pull_request_invalid_action_type(self) -> None: - """Test that events with non-string action are skipped.""" - with self.code_review_setup(), self.tasks(): - event_with_invalid_action = orjson.loads(PULL_REQUEST_OPENED_EVENT_EXAMPLE) - event_with_invalid_action["action"] = 123 - - self._send_webhook_event( - GithubWebhookType.PULL_REQUEST, - orjson.dumps(event_with_invalid_action), - ) - - self.mock_seer.assert_not_called() - def test_pull_request_skips_when_code_review_disabled(self) -> None: """Test that PR events are skipped when code review features are not enabled.""" with self.tasks(): @@ -185,19 +159,6 @@ def test_pull_request_synchronize_action(self) -> None: self.mock_seer.assert_called_once() self.mock_reaction.assert_called_once() - def test_pull_request_invalid_enum_action(self) -> None: - """Test that actions not in PullRequestAction enum are handled gracefully.""" - with self.code_review_setup(), self.tasks(): - event = orjson.loads(PULL_REQUEST_OPENED_EVENT_EXAMPLE) - event["action"] = "future_action_not_in_enum" - - self._send_webhook_event( - GithubWebhookType.PULL_REQUEST, - orjson.dumps(event), - ) - - self.mock_seer.assert_not_called() - def test_pull_request_blocks_draft_for_ready_for_review_action(self) -> None: """Test that draft PRs are blocked for ready_for_review action.""" with self.code_review_setup(), self.tasks(): diff --git a/tests/sentry/seer/code_review/webhooks/test_scm_listeners.py b/tests/sentry/seer/code_review/webhooks/test_scm_listeners.py new file mode 100644 index 00000000000000..9eae71cad7483f --- /dev/null +++ b/tests/sentry/seer/code_review/webhooks/test_scm_listeners.py @@ -0,0 +1,195 @@ +from collections.abc import Generator +from typing import Any +from unittest.mock import MagicMock, patch +from uuid import uuid4 + +import orjson +import pytest + +from sentry.integrations.types import IntegrationProviderSlug +from sentry.scm.types import ( + PullRequestBranch, + PullRequestEvent, + PullRequestEventData, + SubscriptionEvent, +) +from sentry.seer.code_review.webhooks.scm_listeners import ( + WEBHOOK_SEEN_KEY_PREFIX, + handle_pull_request_via_scm_stream, +) +from sentry.testutils.cases import TestCase +from sentry.utils.redis import redis_clusters + + +def _make_subscription_event( + raw_event: dict[str, Any], delivery_id: str | None = None +) -> SubscriptionEvent: + return { + "received_at": 0, + "type": "github", + "event_type_hint": "pull_request", + "event": orjson.dumps(raw_event).decode(), + "extra": {"github_delivery_id": delivery_id}, + "sentry_meta": None, + } + + +def _make_pr_event(raw_event: dict[str, Any], delivery_id: str | None = None) -> PullRequestEvent: + return PullRequestEvent( + action="opened", + pull_request=PullRequestEventData( + id=str(raw_event.get("pull_request", {}).get("number", 1)), + title="Test PR", + description=None, + head=PullRequestBranch(ref="feature", sha="abc123"), + base=PullRequestBranch(ref="main", sha="def456"), + is_private_repo=False, + author=None, + ), + subscription_event=_make_subscription_event(raw_event, delivery_id), + ) + + +class TestScmListeners(TestCase): + """Unit tests for scm_listeners.py — the SCM stream equivalent of test_handlers.py.""" + + INSTALLATION_ID = "99999" + REPO_EXTERNAL_ID = "12345" + + def setUp(self) -> None: + super().setUp() + self.repo = self.create_repo( + project=self.project, + provider="integrations:github", + external_id=self.REPO_EXTERNAL_ID, + ) + + self.mock_integration = MagicMock() + self.mock_integration.id = 123 + self.mock_integration.provider = IntegrationProviderSlug.GITHUB + + self.mock_org_integration = MagicMock() + self.mock_org_integration.organization_id = self.organization.id + + self.mock_contexts = MagicMock() + self.mock_contexts.integration = self.mock_integration + self.mock_contexts.organization_integrations = [self.mock_org_integration] + + self.raw_event = { + "action": "opened", + "installation": {"id": int(self.INSTALLATION_ID)}, + "repository": {"id": int(self.REPO_EXTERNAL_ID)}, + "pull_request": {"number": 1, "draft": False, "user": {"id": 42}}, + } + + @pytest.fixture(autouse=True) + def mock_preflight_allowed(self) -> Generator[None]: + with patch( + "sentry.seer.code_review.webhooks.scm_listeners.CodeReviewPreflightService" + ) as mock_preflight: + mock_preflight.return_value.check.return_value.allowed = True + mock_preflight.return_value.check.return_value.denial_reason = None + mock_preflight.return_value.check.return_value.settings = None + yield + + @patch("sentry.seer.code_review.webhooks.scm_listeners.integration_service") + @patch("sentry.seer.code_review.webhooks.scm_listeners.CodeReviewPreflightService") + def test_skips_github_enterprise_on_prem( + self, mock_preflight: MagicMock, mock_integration_service: MagicMock + ) -> None: + """GitHub Enterprise on-prem webhooks must be skipped — code review is Cloud-only.""" + self.mock_integration.provider = IntegrationProviderSlug.GITHUB_ENTERPRISE + mock_integration_service.organization_contexts.return_value = self.mock_contexts + + handle_pull_request_via_scm_stream(_make_pr_event(self.raw_event)) + + mock_preflight.assert_not_called() + + @patch("sentry.seer.code_review.webhooks.scm_listeners.integration_service") + @patch("sentry.seer.code_review.webhooks.scm_listeners.CodeReviewPreflightService") + def test_skips_when_no_integration( + self, mock_preflight: MagicMock, mock_integration_service: MagicMock + ) -> None: + """When no integration is found for the installation ID, nothing is processed.""" + mock_contexts = MagicMock() + mock_contexts.integration = None + mock_contexts.organization_integrations = [] + mock_integration_service.organization_contexts.return_value = mock_contexts + + handle_pull_request_via_scm_stream(_make_pr_event(self.raw_event)) + + mock_preflight.assert_not_called() + + @patch("sentry.seer.code_review.webhooks.scm_listeners.integration_service") + @patch("sentry.seer.code_review.webhooks.scm_listeners.CodeReviewPreflightService") + def test_processes_github_com( + self, mock_preflight: MagicMock, mock_integration_service: MagicMock + ) -> None: + """GitHub Cloud webhooks trigger a preflight check.""" + mock_integration_service.organization_contexts.return_value = self.mock_contexts + mock_preflight.return_value.check.return_value.allowed = False + mock_preflight.return_value.check.return_value.denial_reason = None + + handle_pull_request_via_scm_stream(_make_pr_event(self.raw_event)) + + mock_preflight.assert_called_once() + + @patch("sentry.seer.code_review.webhooks.scm_listeners.handle_pull_request_event") + @patch("sentry.seer.code_review.webhooks.scm_listeners.integration_service") + def test_webhook_first_time_seen_handler_invoked( + self, mock_integration_service: MagicMock, mock_handler: MagicMock + ) -> None: + """A delivery ID seen for the first time is processed normally.""" + delivery_id = f"seen-success-{uuid4()}" + mock_integration_service.organization_contexts.return_value = self.mock_contexts + + handle_pull_request_via_scm_stream(_make_pr_event(self.raw_event, delivery_id)) + + mock_handler.assert_called_once() + + @patch("sentry.seer.code_review.webhooks.scm_listeners.handle_pull_request_event") + @patch("sentry.seer.code_review.webhooks.scm_listeners.integration_service") + def test_same_delivery_id_second_seen_skipped( + self, mock_integration_service: MagicMock, mock_handler: MagicMock + ) -> None: + """A duplicate delivery ID within the TTL window is dropped after the first.""" + delivery_id = f"seen-sequential-{uuid4()}" + mock_integration_service.organization_contexts.return_value = self.mock_contexts + event = _make_pr_event(self.raw_event, delivery_id) + + handle_pull_request_via_scm_stream(event) + handle_pull_request_via_scm_stream(event) + + assert mock_handler.call_count == 1 + + @patch("sentry.seer.code_review.webhooks.scm_listeners.handle_pull_request_event") + @patch("sentry.seer.code_review.webhooks.scm_listeners.integration_service") + def test_same_delivery_id_after_ttl_expires_handler_invoked_twice( + self, mock_integration_service: MagicMock, mock_handler: MagicMock + ) -> None: + """After TTL expiry the same delivery ID is treated as new and processed again.""" + delivery_id = f"seen-after-ttl-{uuid4()}" + mock_integration_service.organization_contexts.return_value = self.mock_contexts + event = _make_pr_event(self.raw_event, delivery_id) + + handle_pull_request_via_scm_stream(event) + assert mock_handler.call_count == 1 + + # Simulate TTL expiry by manually deleting the key + cluster = redis_clusters.get("default") + cluster.delete(f"{WEBHOOK_SEEN_KEY_PREFIX}{delivery_id}") + + handle_pull_request_via_scm_stream(event) + assert mock_handler.call_count == 2 + + @patch("sentry.seer.code_review.webhooks.scm_listeners.handle_pull_request_event") + @patch("sentry.seer.code_review.webhooks.scm_listeners.integration_service") + def test_missing_delivery_id_handler_invoked( + self, mock_integration_service: MagicMock, mock_handler: MagicMock + ) -> None: + """When delivery_id is absent the dedup check is skipped and the event is processed.""" + mock_integration_service.organization_contexts.return_value = self.mock_contexts + + handle_pull_request_via_scm_stream(_make_pr_event(self.raw_event, delivery_id=None)) + + mock_handler.assert_called_once()