Migrate Seer handlers for code reviews to SCM listeners#112349
Migrate Seer handlers for code reviews to SCM listeners#112349
Conversation
Backend Test FailuresFailures on
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Action handler registration moved to unreliable import location
- I restored the GitHub handler side-effect import to
sentry.integrations.github.__init__and removed the on-demand plugin-client import so action registration occurs reliably at startup.
- I restored the GitHub handler side-effect import to
Or push these changes by commenting:
@cursor push fbedbaec56
Preview (fbedbaec56)
diff --git a/src/sentry/integrations/github/__init__.py b/src/sentry/integrations/github/__init__.py
--- a/src/sentry/integrations/github/__init__.py
+++ b/src/sentry/integrations/github/__init__.py
@@ -1,5 +1,6 @@
from sentry.rules import rules
from .actions.create_ticket import GitHubCreateTicketAction
+from .handlers import GithubActionHandler # noqa: F401
rules.add(GitHubCreateTicketAction)
diff --git a/src/sentry_plugins/github/client.py b/src/sentry_plugins/github/client.py
--- a/src/sentry_plugins/github/client.py
+++ b/src/sentry_plugins/github/client.py
@@ -3,7 +3,6 @@
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 jwtThis Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| from sentry.rules import rules | ||
|
|
||
| from .actions.create_ticket import GitHubCreateTicketAction | ||
| from .handlers import GithubActionHandler # noqa: F401,F403 |
There was a problem hiding this comment.
Action handler registration moved to unreliable import location
High Severity
The GithubActionHandler side-effect import was moved from sentry/integrations/github/__init__.py to sentry_plugins/github/client.py. This new location is only loaded on-demand, preventing the handler from reliably registering during startup. Consequently, GitHub workflow engine actions, like ticket creation, will not function.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2f4ffa0. Configure here.
There was a problem hiding this comment.
This statement is concerning, but importing .integrations.github.handlers in __init__.py is incompatible with importing the listeners statically in .../scm/stream.py. This all boils down to sentry/integrations/jira/client.py calling absolute_url at module level, which requires initialize_app to have been called.
I checked the original stacktrace when action_handler_registry.register was called for GithubActionHandler and made sure that it's still registered "during startup".
It might be worth defining "during startup": I'm talking about the startup of the backend web server. If GithubActionHandler should be registered to the action_handler_registry for other processes, their startup sequence must indeed be modified in a similar way.
The compromise I chose can be revisited by a human with a more effective brain than mine.
| _handle_pr_webhook_for_autofix_processor, | ||
| code_review_handle_webhook_event, | ||
| ) | ||
| WEBHOOK_EVENT_PROCESSORS = (_handle_pr_webhook_for_autofix_processor,) |
There was a problem hiding this comment.
Bug: Seer code review functionality will silently fail upon deployment because the new async processing path is gated by a feature flag that is disabled by default.
Severity: CRITICAL
Suggested Fix
To prevent a silent outage, either remove the feature flag gating produce_event_to_scm_stream to enable the new path by default, or ensure the deployment process strictly requires enabling the sentry.scm.stream.rollout option beforehand. A safer approach would be to make the new path the default and remove the flag dependency.
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/integrations/github/webhook.py#L847
Potential issue: The synchronous webhook processor `code_review_handle_webhook_event` is
removed and replaced with an asynchronous path gated by the `sentry.scm.stream.rollout`
feature flag. This flag is disabled by default. If the PR is deployed without explicitly
enabling this flag, all Seer code review functionality (PR analysis, check run
reactions, etc.) will silently stop working. The webhooks will continue to return
successful HTTP 204 responses, masking the fact that no processing is occurring.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
This statement is true, and was anticipated in bold font in the PR description.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0e25233. Configure here.
|
|
||
| # Code review is only supported on GitHub Cloud, not GitHub Enterprise on-prem. | ||
| if integration.provider == IntegrationProviderSlug.GITHUB_ENTERPRISE: | ||
| return |
There was a problem hiding this comment.
Unreachable GitHub Enterprise provider check is dead code
Low Severity
The check for integration.provider == IntegrationProviderSlug.GITHUB_ENTERPRISE on line 79 is unreachable. The integration_service.organization_contexts call on line 64 already filters integrations to GITHUB.value, so a GitHub Enterprise integration would never be returned, making this check dead code.
Reviewed by Cursor Bugbot for commit 0e25233. Configure here.
| 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, | ||
| ) |
There was a problem hiding this comment.
Do you have ideas on how this will look as we add support for more providers?
|
The SCM team has decided to start with GitLab and do GitHub later. I'm converting this PR to draft until then. |



SCM listeners were introduced by #107441. This PR applies this new pattern to existing webhooks handlers for Seer code reviews.
I chose to delete
handlers.pyand createscm_listeners.pybecause modifyinghandlers.pyin place would have made the diff unreadable (and the naming felt more appropriate).In the new
scm_listeners.py, there is code that does the equivalent of the oldhandlers.py(e.g. preflight and deduplication checks) as expected, and, more surprisingly, a part ofsrc/sentry/integrations/github/webhook.py(e.g. finding the appropriate repo) that's not (yet?) implemented at SCM platform level.I deleted some tests, e.g. in
tests/sentry/seer/code_review/webhooks/test_check_run.py, because these cases are now caught early by the SCM platform's MsgSpec models.Moving
import GithubActionHandler(used for its side-effects) fromsrc/sentry/integrations/github/__init__.pytosrc/sentry_plugins/github/client.pywas required to allow importing.integrations.githubbeforeinitialize_appis called.I was able to test this change end-to-end locally, by creating a pull request on a test repository and observing that the webhook is called, and leaves an 👀 reaction on the new PR. But to do that, I had to temporarily:
src/sentry/scm/private/stream_producer.py(mostly because I don't know how to configure my local env to indeed rollout the SCM stream feature). We need to actually rollout the feature before deploying this PR.src/sentry/seer/code_review/preflight.pybecause my test repo is not enrolled with Seer.delayinproduce_to_listenerinsrc/sentry/scm/private/ipc.pybecause thedelayed function was never called in my dev env. This failed silently, so it's somewhat concerning and needs to be discussed in stand-up (Did the IPC fail? Is the worker even running?).