Conversation
github-actions
Bot
added
the
Scope: Backend
![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 5807553. Configure here.
| ) | ||
|
|
||
| new_notification_message_object = NewIssueAlertNotificationMessage( | ||
| rule_fire_history_id=self.rule_fire_history.id if self.rule_fire_history else None, |
There was a problem hiding this comment.
Validation rejects notifications missing rule_fire_history_id
High Severity
NewIssueAlertNotificationMessage is now created without rule_fire_history_id (always None), but rule_action_uuid is still set. The XNOR validation in get_validation_error requires both fields to be present or both absent. Since rule_fire_history_id is None and rule_action_uuid is not, every call to create_notification_message will raise RuleFireHistoryAndRuleActionUuidActionValidationError, silently preventing all issue alert notification messages from being persisted. The same validation also blocks saves when message_identifier is set.
Reviewed by Cursor Bugbot for commit 5807553. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| new_notification_message_object = NewIssueAlertNotificationMessage( | ||
| rule_fire_history_id=self.rule_fire_history.id if self.rule_fire_history else None, | ||
| rule_action_uuid=rule_action_uuid, | ||
| ) |
There was a problem hiding this comment.
Bug: Removing rule_fire_history_id from NewIssueAlertNotificationMessage construction causes a validation error, silently preventing notification messages from being saved and breaking Slack alert threading.
Severity: HIGH
Suggested Fix
The rule_fire_history_id should be passed during the construction of NewIssueAlertNotificationMessage to satisfy the validation constraint that requires both rule_fire_history_id and rule_action_uuid to be either set or null together.
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/slack/actions/notification.py#L326-L328
Potential issue: The removal of `rule_fire_history_id` when constructing
`NewIssueAlertNotificationMessage` will cause a validation error. The
`get_validation_error()` method enforces a constraint that `rule_fire_history_id` and
`rule_action_uuid` must either both be present or both be null. Since
`rule_fire_history_id` is no longer passed, it defaults to `None` while
`rule_action_uuid` is set, triggering a
`RuleFireHistoryAndRuleActionUuidActionValidationError`. This error is silently caught
in `_save_issue_alert_notification_message`, which prevents the notification message
from being saved. As a result, subsequent alerts cannot find a parent message, breaking
the threading functionality for Slack issue alerts and causing all alerts to be sent as
new messages.
Did we get this right? 👍 / 👎 to inform future reviews.
ceorourke
changed the title
ceorourke
force-pushed
the
ceorourke/rm-rulefirehistory-rules
branch
from
2ccdddc to
5807553
Compare
Remove a lot (but not all) of references to `RuleFireHistory`. This table is completely empty as we don't write to it anymore and it's passed the retention period so the records have been cleaned up. I'm working on removing the remaining references #115035 which are tied to notifications but I need to do a migration to remove the FK on `NotificationMessage` first and this is still a large diff so I chose to keep it separate.
ceorourke
force-pushed
the
ceorourke/rm-rulefirehistory-rules
branch
from
5807553 to
d3b74ce
Compare
| open_period_start: datetime | None = None, | ||
| ) -> IssueAlertNotificationMessage | None: | ||
| """ | ||
| Returns the parent notification message for a metric rule if it exists, otherwise returns None. |
There was a problem hiding this comment.
This docstring says "metric rule" but this code was for issue alerts - note the rule_id, rule_action_uuid, and logger message.
1 participant
Remove all references to
RuleFireHistoryto prepare to fully remove the model. Note that this table is completely empty and is not being written to anymore.NotificationMessage's columnsrule_fire_historyandrule_action_uuidwhich were used for issue alerts are all null currently and are not used to create threads - rather we have workflow engine code that handles that instead.