Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/sentry/integrations/github/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
from collections.abc import Mapping, Sequence
from datetime import datetime, timedelta
from enum import StrEnum
from typing import Any, TypedDict

import orjson
Expand Down Expand Up @@ -59,6 +60,21 @@ def __repr__(self) -> str:
return f"GithubRateLimitInfo(limit={self.limit},rem={self.remaining},reset={self.reset})"


class GitHubReaction(StrEnum):
"""
https://docs.github.com/en/rest/reactions/reactions#about-reactions
"""

PLUS_ONE = "+1"
MINUS_ONE = "-1"
LAUGH = "laugh"
CONFUSED = "confused"
HEART = "heart"
HOORAY = "hooray"
ROCKET = "rocket"
EYES = "eyes"


class GithubSetupApiClient(IntegrationProxyClient):
"""
API Client that doesn't require an installation.
Expand Down Expand Up @@ -585,6 +601,18 @@ def get_comment_reactions(self, repo: str, comment_id: str) -> Any:
reactions.pop("url", None)
return reactions

def create_comment_reaction(self, repo: str, comment_id: str, reaction: GitHubReaction) -> Any:
"""
https://docs.github.com/en/rest/reactions/reactions#create-reaction-for-an-issue-comment

Args:
repo: Repository name in "owner/repo" format
comment_id: The ID of the comment
reaction: The reaction type
"""
endpoint = f"/repos/{repo}/issues/comments/{comment_id}/reactions"
return self.post(endpoint, data={"content": reaction.value})

def get_user(self, gh_username: str) -> Any:
"""
https://docs.github.com/en/rest/users/users#get-a-user
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/integrations/github/webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ def _handle(
event=event,
organization=organization,
repo=repo,
integration=integration,
**kwargs,
)
except Exception as e:
Expand Down Expand Up @@ -992,8 +993,7 @@ class IssueCommentEventWebhook(GitHubWebhook):
"""

EVENT_TYPE = IntegrationWebhookEventType.ISSUE_COMMENT
# XXX: Once we port the Overwatch feature, we can add the processor here.
WEBHOOK_EVENT_PROCESSORS = ()
WEBHOOK_EVENT_PROCESSORS = (code_review_handle_webhook_event,)


@all_silo_endpoint
Expand Down
34 changes: 22 additions & 12 deletions src/sentry/seer/code_review/webhooks/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,20 @@
if TYPE_CHECKING:
from sentry.integrations.github.webhook import WebhookProcessor

from ..permissions import has_code_review_enabled
from ..utils import _transform_webhook_to_codegen_request
from .check_run import handle_check_run_event
from .issue_comment import handle_issue_comment_event

logger = logging.getLogger(__name__)

METRICS_PREFIX = "seer.code_review.webhook"


def handle_other_webhook_event(
*,
event_type: str,
event: Mapping[str, Any],
organization: Organization,
repo: Repository,
**kwargs: Any,
def _schedule_task(
event_type: str, event: Mapping[str, Any], organization: Organization, repo: Repository
) -> None:
"""
Each webhook event type may implement its own handler.
This is a generic handler for non-PR-related events (e.g., issue_comment on regular issues).
"""
"""Transform and forward a webhook event to Seer for processing."""
from .task import process_github_webhook_event

event_type_enum = EventType.from_string(event_type)
Expand All @@ -58,9 +52,24 @@ def handle_other_webhook_event(
)


def handle_other_webhook_event(
*,
event_type: str,
event: Mapping[str, Any],
organization: Organization,
repo: Repository,
**kwargs: Any,
) -> None:
"""Handle other webhook events (pull_request, pull_request_review, etc.)."""
if not has_code_review_enabled(organization):
return

_schedule_task(event_type, event, organization, repo)


EVENT_TYPE_TO_handler: dict[EventType, WebhookProcessor] = {
EventType.CHECK_RUN: handle_check_run_event,
EventType.ISSUE_COMMENT: handle_other_webhook_event,
EventType.ISSUE_COMMENT: handle_issue_comment_event,
EventType.PULL_REQUEST: handle_other_webhook_event,
EventType.PULL_REQUEST_REVIEW: handle_other_webhook_event,
EventType.PULL_REQUEST_REVIEW_COMMENT: handle_other_webhook_event,
Expand Down Expand Up @@ -98,5 +107,6 @@ def handle_webhook_event(


# Type checks to ensure the functions match WebhookProcessor protocol
_type_checked_handle_issue_comment_event: WebhookProcessor = handle_issue_comment_event
_type_checked_handle_other_webhook_event: WebhookProcessor = handle_other_webhook_event
_type_checked_handle_check_run_event: WebhookProcessor = handle_check_run_event
114 changes: 114 additions & 0 deletions src/sentry/seer/code_review/webhooks/issue_comment.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
"""
Handler for GitHub issue_comment webhook events.
"""

from __future__ import annotations

import enum
import logging
from collections.abc import Mapping
from typing import Any

from sentry import options
from sentry.integrations.github.client import GitHubReaction
from sentry.integrations.services.integration import RpcIntegration
from sentry.models.organization import Organization
from sentry.models.repository import Repository
from sentry.utils import metrics

from ..permissions import has_code_review_enabled

logger = logging.getLogger(__name__)


class ErrorStatus(enum.StrEnum):
MISSING_INTEGRATION = "missing_integration"
REACTION_FAILED = "reaction_failed"


class Log(enum.StrEnum):
MISSING_INTEGRATION = "github.webhook.issue_comment.missing-integration"
REACTION_FAILED = "github.webhook.issue_comment.reaction-failed"


class Metrics(enum.StrEnum):
ERROR = "seer.code_review.webhook.issue_comment.error"
OUTCOME = "seer.code_review.webhook.issue_comment.outcome"


SENTRY_REVIEW_COMMAND = "@sentry review"


def is_pr_review_command(comment_body: str | None) -> bool:
if comment_body is None:
return False
return SENTRY_REVIEW_COMMAND in comment_body.lower()


def _add_eyes_reaction_to_comment(
integration: RpcIntegration | None,
organization: Organization,
repo: Repository,
comment_id: str,
) -> None:
extra = {"organization_id": organization.id, "repo": repo.name, "comment_id": comment_id}

if integration is None:
metrics.incr(
Metrics.ERROR.value,
tags={"error_status": ErrorStatus.MISSING_INTEGRATION.value},
)
logger.warning(
Log.MISSING_INTEGRATION.value,
extra=extra,
)
return

try:
client = integration.get_installation(organization_id=organization.id).get_client()
client.create_comment_reaction(repo.name, comment_id, GitHubReaction.EYES)
metrics.incr(
Metrics.OUTCOME.value,
tags={"status": "reaction_added"},
)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can either let the exception raise and handle it in the caller or you need to return the outcome to the caller and exit.

Image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this one, I was purposely having the helper _add_eyes_reaction_to_comment log and swallow the exception so every caller of the helper wouldn't need to wrap in a try/catch. I would like us to still forward to seer even if for some reason the 👀 posting failed. For example if we hit a rate limit here, but still want the PR review itself to proceed. What do you think?

metrics.incr(
Metrics.ERROR.value,
tags={"error_status": ErrorStatus.REACTION_FAILED.value},
)
logger.exception(
Log.REACTION_FAILED.value,
extra=extra,
)


def handle_issue_comment_event(
*,
event_type: str,
event: Mapping[str, Any],
organization: Organization,
repo: Repository,
integration: RpcIntegration | None = None,
**kwargs: Any,
) -> None:
"""
Handle issue_comment webhook events for PR review commands.
"""
comment = event.get("comment", {})
comment_id = comment.get("id")
comment_body = comment.get("body")

if not has_code_review_enabled(organization):
return

if not is_pr_review_command(comment_body or ""):
return

if comment_id:
if not options.get("github.webhook.issue-comment"):
_add_eyes_reaction_to_comment(integration, organization, repo, str(comment_id))

# Import here to avoid circular dependency with handlers
from .handlers import _schedule_task

_schedule_task(event_type, event, organization, repo)
Comment on lines +107 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The _schedule_task function is called unconditionally, causing duplicate processing when the github.webhook.issue-comment option is True.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

In issue_comment.py, the code checks the github.webhook.issue-comment option. If this option is False, it adds a reaction. However, the call to _schedule_task on line 114 occurs outside of this conditional check, meaning it is executed unconditionally. This causes a task to be scheduled in Sentry even when the option is True, a state intended to forward the event to another service (Overwatch). This results in the event being processed by both Sentry and the other service, leading to conflicting behavior.

💡 Suggested Fix

Move the _schedule_task(...) call inside the if not options.get("github.webhook.issue-comment"): block to ensure it is only executed when this option is False.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/seer/code_review/webhooks/issue_comment.py#L107-L114

Potential issue: In `issue_comment.py`, the code checks the
`github.webhook.issue-comment` option. If this option is `False`, it adds a reaction.
However, the call to `_schedule_task` on line 114 occurs outside of this conditional
check, meaning it is executed unconditionally. This causes a task to be scheduled in
Sentry even when the option is `True`, a state intended to forward the event to another
service (Overwatch). This results in the event being processed by both Sentry and the
other service, leading to conflicting behavior.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8054017

25 changes: 24 additions & 1 deletion tests/sentry/integrations/github/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

from sentry.constants import ObjectStatus
from sentry.integrations.github.blame import create_blame_query, generate_file_path_mapping
from sentry.integrations.github.client import GitHubApiClient
from sentry.integrations.github.client import GitHubApiClient, GitHubReaction
from sentry.integrations.github.integration import GitHubIntegration
from sentry.integrations.source_code_management.commit_context import (
CommitInfo,
Expand Down Expand Up @@ -410,6 +410,29 @@ def test_get_comment_reactions_missing_reactions(self, get_jwt) -> None:
reactions = self.github_client.get_comment_reactions(repo=self.repo.name, comment_id="2")
assert reactions == {}

@mock.patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1")
@responses.activate
def test_create_comment_reaction(self, get_jwt) -> None:
response_data = {
"id": 1,
"node_id": "MDg6UmVhY3Rpb24x",
"user": {"login": "octocat", "id": 1},
"content": "eyes",
"created_at": "2016-05-20T20:09:31Z",
}
responses.add(
responses.POST,
f"https://api.github.com/repos/{self.repo.name}/issues/comments/123/reactions",
json=response_data,
status=201,
)

result = self.github_client.create_comment_reaction(
repo=self.repo.name, comment_id="123", reaction=GitHubReaction.EYES
)
assert result == response_data
assert orjson.loads(responses.calls[0].request.body) == {"content": "eyes"}

@mock.patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1")
@responses.activate
def test_get_merge_commit_sha_from_commit(self, get_jwt) -> None:
Expand Down
Loading
Loading