ref(webhooks): Add legacy_webhook to the Plugin ActionType#116454
Conversation
Mirror the WebhookActionHandler dual-path pattern so PLUGIN actions also route through the new task-based legacy webhook service when the existing legacy-webhook feature flags are enabled. Without flags, behavior is unchanged (old synchronous path only). This ensures projects using the PLUGIN action type (from dual-written 'notify all legacy integrations' rules) benefit from the new service's retry logic, metrics, and feature-flagged control. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
github-actions
Bot
added
the
Scope: Backend
Christinarlong
changed the title
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
The old path (execute_via_group_type_registry) notifies all legacy NotificationPlugins, not just webhooks. Disabling it entirely would break Slack, PagerDuty, OpsGenie, and other plugins still in use. Instead, always run the old path but skip the webhooks plugin in NotifyEventAction.get_plugins() when the disable-old-path flag is on. The new task-based service handles webhook delivery exclusively. Co-Authored-By: Claude <noreply@anthropic.com>
| if skip_webhooks and plugin.slug == "webhooks": | ||
| continue |
There was a problem hiding this comment.
we can't cut off at the action layer as we still need to alert for the following notificationplugins (that aren't slack, pagerduty, webhooks, and opsgenie)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
leeandher
left a comment
leeandher
left a comment
There was a problem hiding this comment.
Makes sense! I did notice that we're using two flags, one to shut off the old path, one to enable the new path -- It just makes a lot more possibilities to consider to avoid putting the user in a bad state but I suspect it's done for a reason, just mentioning it in case theres a way around it
| if new_path and isinstance(invocation.event_data.event, GroupEvent): | ||
| send_legacy_webhooks_for_invocation(invocation) |
There was a problem hiding this comment.
Does the old path even permit Activity? It may be worth adding this GroupEvent gate right at the top and having the only condition to check here by the feature flag.
| except Exception: | ||
| logger.exception( | ||
| "plugin_action_handler.old_path_error", | ||
| extra={"invocation": invocation}, |
There was a problem hiding this comment.
nit: I'd add the same error handling for your new path if you want to compare logging, but if you don't expect errors it might be fine
Move the GroupEvent isinstance check to the top of execute() as an early return since both paths require it. Add try/except error handling around the new path for logging parity with the old path. Co-Authored-By: Claude <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b841095. Configure here.
| def execute(invocation: ActionInvocation) -> None: | ||
| execute_via_group_type_registry(invocation) | ||
| if not isinstance(invocation.event_data.event, GroupEvent): | ||
| return |
There was a problem hiding this comment.
GroupEvent gate blocks Activity events from old path
High Severity
The early return for non-GroupEvent events prevents Activity events from reaching execute_via_group_type_registry, which explicitly handles Activity events (e.g., calling send_notification() for resolution notifications). Before this change, the old path ran unconditionally. The analogous WebhookActionHandler.execute() correctly places the GroupEvent check only on the new path, not the old one. This silently drops plugin notifications for Activity-based events like issue resolutions.
Reviewed by Cursor Bugbot for commit b841095. Configure here.
There was a problem hiding this comment.
plugins don't support activity alerts since there's no metric handler
2 participants
Turns out webhooks can also be sent from the PLUGIN ActionType via "Notify all legacy integrations" action option. So add calling into the legacy_webhook service from here since we don't care about notifying the other plugins