feat(workflow_engine): Add in hook for producing occurrences from the stateful detector#1
Conversation
… stateful detector (#80168) This adds a hook that can be implemented to produce an occurrence specific to the detector that is subclassing the StatefulDetector. Also change the signature of evaluate to return a dict keyed by groupkey instead of a list. This helps avoid the chance of duplicate results for the same group key. <!-- Describe your PR here. -->
🤖 Grapple PR ReviewSummaryThis PR refactors the stateful detector's evaluation pipeline to return a dict keyed by DetectorGroupKey instead of a list, and adds an abstract Changes: 4 files (+249 / -151 lines)
CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 4 files"/]
f1["🟡 grouptype.py +3/-8"]
pr_0 --> f1
f2["🟡 detector.py +7/-1"]
pr_0 --> f2
f3["🟡 detector.py +22/-23"]
pr_0 --> f3
f4["🟡 test_detector.py +217/-119"]
pr_0 --> f4
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
|
CRITICAL — MetricAlertDetectorHandler extends StatefulDetectorHandler but only contains |
|
CRITICAL — MetricAlertDetectorHandler inherits from StatefulDetectorHandler but does not implement the abstract method |
|
MAJOR — PriorityLevel vs DetectorPriorityLevel type mismatch at the call site. |
|
MAJOR — Type inconsistency between PriorityLevel and DetectorPriorityLevel. The |
|
MAJOR — The return type of |
|
MINOR — If |
|
MINOR — In |
|
MINOR — The duplicate group key detection and error logging has been removed. While the dict structure prevents duplicates by design, the old logging served as a canary for buggy handler implementations that produce the same key twice. With a dict, the second value silently overwrites the first. Consider adding a debug-level log or metric if a handler's evaluate() produces results that would collide. |
|
MINOR — The Detector model now imports |
|
MINOR — Importing |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a hook for producing issue occurrences from the stateful detector by introducing an abstract build_occurrence_and_event_data method to StatefulDetectorHandler, changing the evaluate return type from list[DetectorEvaluationResult] to dict[DetectorGroupKey, DetectorEvaluationResult], and exposing group_type as a reusable property on the Detector model.
Changes: 4 files (+249 / -151 lines)
- 🟡 4 files modified:
src/sentry/incidents/grouptype.py,src/sentry/workflow_engine/models/detector.py,src/sentry/workflow_engine/processors/detector.py,tests/sentry/workflow_engine/processors/test_detector.py
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 4 files"/]
f1["🟡 grouptype.py +3/-8"]
pr_0 --> f1
f2["🟡 detector.py +7/-1"]
pr_0 --> f2
f3["🟡 detector.py +22/-23"]
pr_0 --> f3
f4["🟡 test_detector.py +217/-119"]
pr_0 --> f4
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 5 |
| 🟡 Minor | 3 |
| 🔵 Info | 4 |
Review details
- Reviewed at: 2026-04-07T21:08:01.173Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 76.1s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| from sentry.workflow_engine.processors.detector import StatefulDetectorHandler | ||
|
|
||
|
|
||
| # TODO: This will be a stateful detector when we build that abstraction |
There was a problem hiding this comment.
🔴 CRITICAL — Business Logic Correctness (confidence: 100%)
MetricAlertDetectorHandler inherits from StatefulDetectorHandler but does not implement the abstract methods get_dedupe_value, get_group_key_values, or build_occurrence_and_event_data. Since StatefulDetectorHandler declares these as @abc.abstractmethod, Python will raise TypeError: Can't instantiate abstract class MetricAlertDetectorHandler with abstract methods ... when any code attempts to instantiate this class. The intent specification acknowledges this risk but the acceptance criteria state 'MetricAlertDetectorHandler passes through to StatefulDetectorHandler.evaluate() correctly with no override needed', which is impossible without implementing the abstract methods.
Evidence:
- StatefulDetectorHandler declares abstract methods: get_dedupe_value, get_group_key_values, build_occurrence_and_event_data
- MetricAlertDetectorHandler is defined as
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): pass - Python ABC enforcement will prevent instantiation of this class
- Intent spec risk area: 'MetricAlertDetectorHandler inheriting StatefulDetectorHandler without implementing build_occurrence_and_event_data — this is an abstract method and the class will fail to instantiate in production until implemented'
Agent: logic
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): | ||
| pass |
There was a problem hiding this comment.
🟠 MAJOR — Authentication/Authorization (confidence: 100%)
MetricAlertDetectorHandler now inherits from StatefulDetectorHandler but does not implement the abstract method build_occurrence_and_event_data. This means any attempt to instantiate MetricAlertDetectorHandler in production will raise a TypeError, effectively breaking metric alert detection. Until implemented, no metric alert occurrences can be produced.
Evidence:
- StatefulDetectorHandler declares
build_occurrence_and_event_dataas an@abc.abstractmethod. - MetricAlertDetectorHandler only has
pass— no implementation of build_occurrence_and_event_data. - Python's ABC enforcement means MetricAlertDetectorHandler cannot be instantiated, raising TypeError at runtime.
- The PR description acknowledges this as a known edge case ('MetricAlertDetectorHandler with no override of build_occurrence_and_event_data — will raise NotImplementedError or TypeError since it's abstract; this must be implemented before production use'), but the class is merged without a guard or NotImplementedError with a descriptive message.
Agent: security
|
|
||
| # TODO: Add hook here for generating occurrence | ||
| else: | ||
| result, event_data = self.build_occurrence_and_event_data( |
There was a problem hiding this comment.
🟠 MAJOR — Error Handling (confidence: 100%)
When new_status != DetectorPriorityLevel.OK, build_occurrence_and_event_data is called but there is no try/except around it. If a subclass's implementation raises an exception, it will propagate up and abort the entire detector evaluation for all remaining group keys. The intent spec edge case explicitly asks: 'What happens when build_occurrence_and_event_data raises an exception — is it caught or does it propagate and abort the detector evaluation?' The answer is it propagates and aborts, which could cause data loss for other group keys in the same evaluation batch.
Evidence:
- The for loop in evaluate() at line 238 iterates over all group_values
- evaluate_group_key_value calls build_occurrence_and_event_data without try/except
- An exception in build_occurrence_and_event_data for one group key would prevent processing of subsequent group keys
Agent: logic
|
|
||
| for result in detector_results.values(): | ||
| if result.result is not None: | ||
| create_issue_occurrence_from_result(result) |
There was a problem hiding this comment.
🟠 MAJOR — Edge Cases (confidence: 95%)
Empty dict returned from evaluate() is falsy in Python (bool({}) == False), so if detector_results: on line 62 will skip appending to results when evaluate() returns an empty dict. This matches the behavior for empty lists, but differs from the previous behavior where an empty list would also be skipped. This is correct behavior per the intent spec edge case, but worth noting that this is consistent.
Evidence:
- Python:
bool({}) == False - Line 62:
if detector_results:will correctly skip empty dicts
Agent: logic
| if result: | ||
| results.append(result) | ||
| results[result.group_key] = result | ||
| return results |
There was a problem hiding this comment.
🟠 MAJOR — Intent Alignment - Silent Overwrite (confidence: 90%)
The previous code logged an error when duplicate group keys were detected. The dict structure now silently overwrites earlier results for the same key. While the intent spec acknowledges removing the duplicate detection, this is identified as a risk area: 'any bugs in evaluate() returning conflicting keys will now silently overwrite earlier results rather than logging an error'. Since group_values.items() from get_group_key_values returns unique keys by nature of being a dict, duplicates can't occur within a single evaluate() call. However, if evaluate_group_key_value returns a result with a different group_key than the iteration key, the dict would store it under result.group_key rather than the iteration key, potentially causing unexpected behavior.
Evidence:
- Line 243:
results[result.group_key] = resultuses the result's group_key, not the iteration variablegroup_key - If evaluate_group_key_value ever returns a result with a different group_key than what was passed in, this could cause mismatches
Agent: logic
| @abc.abstractmethod | ||
| def build_occurrence_and_event_data( | ||
| self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel | ||
| ) -> tuple[IssueOccurrence, dict[str, Any]]: |
There was a problem hiding this comment.
🟡 MINOR — Input Validation (confidence: 93%)
build_occurrence_and_event_data receives value: int and new_status: PriorityLevel with no validation or exception handling. If a subclass implementation raises an exception (e.g., due to an invalid group_key or unexpected value), it will propagate uncaught through evaluate_group_key_value and evaluate(), aborting the entire detector evaluation loop for all group keys.
Evidence:
- The call site in evaluate_group_key_value (lines ~295-298) has no try/except around
self.build_occurrence_and_event_data(group_key, value, PriorityLevel(new_status)). - PriorityLevel(new_status) can raise ValueError if new_status is not a valid PriorityLevel member — this is an integer cast from DetectorPriorityLevel which may not always map cleanly.
- An exception here would abort processing for all remaining group_key iterations in the outer for loop in evaluate(), meaning partial failures silently drop results for other keys.
- The PR description notes this edge case: 'What happens when build_occurrence_and_event_data raises an exception — is it caught or does it propagate and abort the detector evaluation?'
Agent: security
| from sentry.workflow_engine.processors.detector import StatefulDetectorHandler | ||
|
|
||
|
|
||
| # TODO: This will be a stateful detector when we build that abstraction |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 100%)
MetricAlertDetectorHandler now inherits from StatefulDetectorHandler and has an empty body, but does not implement the required abstract method build_occurrence_and_event_data(). This will cause a TypeError at instantiation time. Either the method must be implemented or a comment should explain that this is intentionally left for future implementation.
Evidence:
- Line 9: Class inherits from
StatefulDetectorHandler - Lines 10: Empty
passbody - StatefulDetectorHandler (detector.py line 166) declares
build_occurrence_and_event_dataas@abc.abstractmethod - No implementation of
build_occurrence_and_event_datain MetricAlertDetectorHandler
Agent: style
| @@ -9,6 +10,7 @@ | |||
| from sentry.backup.scopes import RelocationScope | |||
| from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model | |||
| from sentry.issues import grouptype | |||
There was a problem hiding this comment.
🔵 INFO — Dependency risks (confidence: 99%)
import builtins is used solely to reference builtins.type[GroupType] in the type annotation. While not a vulnerability, this is an unusual pattern that could cause confusion and is unnecessary — Python's built-in type is accessible directly in type annotations using type[GroupType] or Type[GroupType] from typing.
Evidence:
import builtinsis added and used only in the return annotationbuiltins.type[GroupType] | None.- The standard approach is
from typing import Typeand useType[GroupType], or in Python 3.9+ usetype[GroupType]directly without importing builtins. - Importing builtins explicitly is not harmful but is non-idiomatic and could be surprising to reviewers or lead to confusion about intent.
Agent: security
| @@ -9,6 +10,7 @@ | |||
| from sentry.backup.scopes import RelocationScope | |||
There was a problem hiding this comment.
🔵 INFO — Naming conventions (confidence: 89%)
Import statement import builtins is used only to reference builtins.type[GroupType] in the return type annotation. This is unconventional; the standard practice is to use type[GroupType] directly without the builtins. prefix, as type is a builtin.
Evidence:
- Line 8:
import builtinsadded - Line 58:
builtins.type[GroupType] | Noneuses the full qualified name - The
typebuiltin does not need to be qualified withbuiltins.
Agent: style
| results = {} | ||
| for group_key, group_value in group_values.items(): | ||
| result = self.evaluate_group_key_value( | ||
| group_key, group_value, all_state_data[group_key], dedupe_value |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 84%)
The docstring for StatefulDetectorHandler.evaluate() still references returning 'a list of DetectorEvaluationResult' but the method now returns a dict. The docstring is outdated and should be updated to reflect the new return type.
Evidence:
- Line 225: Method signature returns
dict[DetectorGroupKey, DetectorEvaluationResult] - Line 227: Docstring says 'returns a list of
DetectorEvaluationResult' - Lines 230-231: Old docstring text about deduping 'values from a list' is no longer applicable
Agent: style
|
|
||
| for result in detector_results.values(): | ||
| if result.result is not None: | ||
| create_issue_occurrence_from_result(result) |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))
Empty dict returned from evaluate() is falsy in Python (
bool({}) == False), soif detector_results:on line 62 will skip appending to results when evaluate() returns an empty dict. This matches the behavior for empty lists, but differs from the previous behavior where an empty list would also be skipped. This is correct behavior per the intent spec edge case, but worth noting that this is consistent.
| create_issue_occurrence_from_result(result) | |
| # NOTE: detector_results is a dict[DetectorGroupKey, DetectorEvaluationResult]. | |
| # An empty dict is falsy in Python (bool({}) == False), so the checks below | |
| # correctly skip processing and appending when there are no results. | |
| # This is intentional — it mirrors the previous list-based behavior. | |
| # Only append detectors that produced at least one result (non-empty dict). |
🤖 Grapple PR auto-fix • major • confidence: 95%
| if result: | ||
| results.append(result) | ||
| results[result.group_key] = result | ||
| return results |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (10 lines, 1 file))
The previous code logged an error when duplicate group keys were detected. The dict structure now silently overwrites earlier results for the same key. While the intent spec acknowledges removing the duplicate detection, this is identified as a risk area: 'any bugs in evaluate() returning conflicting keys will now silently overwrite earlier results rather than logging an error'. Since
group_values.items()fromget_group_key_valuesreturns unique keys by nature of being a dict, duplicates can't occur within a single evaluate() call. However, ifevaluate_group_key_valuereturns a result with a differentgroup_keythan the iteration key, the dict would store it underresult.group_keyrather than the iteration key, potentially causing unexpected behavior.
| return results | |
| if result.group_key != group_key: | |
| logger.error( | |
| "evaluate_group_key_value returned result with mismatched group_key", | |
| extra={ | |
| "expected_group_key": group_key, | |
| "result_group_key": result.group_key, | |
| }, | |
| ) | |
| results[group_key] = result |
🤖 Grapple PR auto-fix • major • confidence: 90%
| from sentry.workflow_engine.processors.detector import StatefulDetectorHandler | ||
|
|
||
|
|
||
| # TODO: This will be a stateful detector when we build that abstraction |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (11 lines, 1 file) — review recommended)
MetricAlertDetectorHandler inherits from StatefulDetectorHandler but does not implement the abstract methods
get_dedupe_value,get_group_key_values, orbuild_occurrence_and_event_data. Since StatefulDetectorHandler declares these as@abc.abstractmethod, Python will raiseTypeError: Can't instantiate abstract class MetricAlertDetectorHandler with abstract methods ...when any code attempts to instantiate this class. The intent specification acknowledges this risk but the acceptance criteria state 'MetricAlertDetectorHandler passes through to StatefulDetectorHandler.evaluate() correctly with no override needed', which is impossible without implementing the abstract methods.
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -4,10 +4,13 @@
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
-from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
+from sentry.workflow_engine.models import DataPacket
+from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
-class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]):
+ def evaluate(
+ self, data_packet: DataPacket[QuerySubscriptionUpdate]
+ ) -> list[DetectorEvaluationResult]:
+ # TODO: Implement when StatefulDetectorHandler abstract methods are ready
+ return []🤖 Grapple PR auto-fix • critical • Review this diff before applying
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): | ||
| pass |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (14 lines, 1 file) — review recommended)
MetricAlertDetectorHandler now inherits from StatefulDetectorHandler but does not implement the abstract method build_occurrence_and_event_data. This means any attempt to instantiate MetricAlertDetectorHandler in production will raise a TypeError, effectively breaking metric alert detection. Until implemented, no metric alert occurrences can be produced.
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -1,12 +1,23 @@
from dataclasses import dataclass
+from typing import Any
+
from sentry.incidents.utils.types import QuerySubscriptionUpdate
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+ def build_occurrence_and_event_data(
+ self, *args: Any, **kwargs: Any
+ ) -> Any:
+ # TODO: Implement build_occurrence_and_event_data for metric alert detection.
+ # This must be completed before MetricAlertDetectorHandler can be used in production.
+ # See StatefulDetectorHandler.build_occurrence_and_event_data for the expected signature
+ # and return type.
+ raise NotImplementedError(
+ "MetricAlertDetectorHandler.build_occurrence_and_event_data is not yet implemented. "
+ "This must be implemented before metric alert occurrences can be produced."
+ )🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| # TODO: Add hook here for generating occurrence | ||
| else: | ||
| result, event_data = self.build_occurrence_and_event_data( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (17 lines, 1 file) — review recommended)
When
new_status != DetectorPriorityLevel.OK,build_occurrence_and_event_datais called but there is no try/except around it. If a subclass's implementation raises an exception, it will propagate up and abort the entire detector evaluation for all remaining group keys. The intent spec edge case explicitly asks: 'What happens when build_occurrence_and_event_data raises an exception — is it caught or does it propagate and abort the detector evaluation?' The answer is it propagates and aborts, which could cause data loss for other group keys in the same evaluation batch.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -293,7 +293,18 @@ def evaluate_group_key_value(
else:
- result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
- )
+ try:
+ result, event_data = self.build_occurrence_and_event_data(
+ group_key, value, PriorityLevel(new_status)
+ )
+ except Exception:
+ logger.exception(
+ "Failed to build occurrence and event data for detector",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": group_key,
+ "new_status": new_status,
+ },
+ )
+ return None
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| # TODO: Add hook here for generating occurrence | ||
| else: | ||
| result, event_data = self.build_occurrence_and_event_data( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (12 lines, 1 file) — review recommended)
The
valueparameter passed tobuild_occurrence_and_event_datais the originalvaluevariable from the method parameter, notgroup_valuefrom the condition evaluation. Looking at the method signatureevaluate_group_key_value(self, group_key, group_value, state_data, dedupe_value), the parameter is namedvalueat line 265. This is correct but could be confusing since the tests callbuild_mock_occurrence_and_eventwith hardcoded value6even when the actual group_value is different (e.g., 100 or 200 in test_results_on_change).
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -263,7 +263,7 @@ class StatefulDetectorHandler(DetectorHandler[T], abc.ABC):
def evaluate_group_key_value(
- self, group_key, value, state_data, dedupe_value
+ self, group_key, group_value, state_data, dedupe_value
) -> DetectorEvaluationResult | None:
"""
Evaluates a single group key value against its state data and returns
a DetectorEvaluationResult if any state changes are detected.
"""
- if not self.conditions or not any(
- condition.is_triggered(value) for condition in self.conditions
- ):
+ if not self.conditions or not any(
+ condition.is_triggered(group_value) for condition in self.conditions
+ ):
return None
state_data = state_data or DetectorStateData(
group_key=group_key,
is_active=False,
status=DetectorPriorityLevel.OK,
dedupe_value=None,
counter_updates={},
)
- new_status = self.get_new_status(state_data, value)
+ new_status = self.get_new_status(state_data, group_value)
if new_status is None or new_status == state_data.status:
return None
if dedupe_value <= (state_data.dedupe_value or 0):
return None
is_active = new_status != DetectorPriorityLevel.OK
self.enqueue_state_update(group_key, is_active, new_status)
event_data = None
result: StatusChangeMessage | IssueOccurrence
if new_status == DetectorPriorityLevel.OK:
result = StatusChangeMessage(
fingerprint=self.build_fingerprint(group_key),
project_id=self.detector.project_id,
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)
else:
result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
+ group_key, group_value, PriorityLevel(new_status)
)
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,
priority=new_status,
result=result,
event_data=event_data,
)
return None🤖 Grapple PR auto-fix • minor • Review this diff before applying
| result, event_data = self.build_occurrence_and_event_data( | ||
| group_key, value, PriorityLevel(new_status) | ||
| ) | ||
| return DetectorEvaluationResult( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (12 lines, 1 file) — review recommended)
Variable
resultis typed asStatusChangeMessage | IssueOccurrencebut the code path fornew_status == DetectorPriorityLevel.OKassigns aStatusChangeMessage, while the else branch assigns anIssueOccurrence. The type annotation is imprecise and should reflect the actual assignment. Additionally,event_datais assigned only in the else branch but used unconditionally in the return statement, which could lead to an undefined variable error.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -286,16 +286,17 @@ def evaluate_group_key_value(
is_active = new_status != DetectorPriorityLevel.OK
self.enqueue_state_update(group_key, is_active, new_status)
- event_data = None
- result: StatusChangeMessage | IssueOccurrence
- if new_status == DetectorPriorityLevel.OK:
+ result: StatusChangeMessage | IssueOccurrence
+ if new_status == DetectorPriorityLevel.OK:
# If we've determined that we're now ok, we just want to resolve the issue
result = StatusChangeMessage(
fingerprint=self.build_fingerprint(group_key),
project_id=self.detector.project_id,
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)
-
- # TODO: Add hook here for generating occurrence
+ event_data = None
+ else:
+ result, event_data = self.build_occurrence_and_event_data(
+ group_key, value, PriorityLevel(new_status)
+ )
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,🤖 Grapple PR auto-fix • minor • Review this diff before applying
| @abc.abstractmethod | ||
| def build_occurrence_and_event_data( | ||
| self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel | ||
| ) -> tuple[IssueOccurrence, dict[str, Any]]: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (30 lines, 1 file) — review recommended)
build_occurrence_and_event_data receives
value: intandnew_status: PriorityLevelwith no validation or exception handling. If a subclass implementation raises an exception (e.g., due to an invalid group_key or unexpected value), it will propagate uncaught through evaluate_group_key_value and evaluate(), aborting the entire detector evaluation loop for all group keys.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -290,8 +290,22 @@ def evaluate_group_key_value(
else:
- result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
- )
+ try:
+ priority_level = PriorityLevel(new_status)
+ except ValueError:
+ logger.exception(
+ "Failed to convert DetectorPriorityLevel to PriorityLevel",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": group_key,
+ "new_status": new_status,
+ },
+ )
+ return None
+ try:
+ result, event_data = self.build_occurrence_and_event_data(
+ group_key, value, priority_level
+ )
+ except Exception:
+ logger.exception(
+ "Failed to build occurrence and event data",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": group_key,
+ "value": value,
+ "new_status": new_status,
+ },
+ )
+ return None🤖 Grapple PR auto-fix • minor • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR refactors the stateful detector evaluation pipeline to return a dict keyed by DetectorGroupKey instead of a list, and adds an abstract build_occurrence_and_event_data hook that subclasses must implement to produce IssueOccurrences from detector results. It also promotes MetricAlertDetectorHandler to use StatefulDetectorHandler directly.
Changes: 4 files (+249 / -151 lines)
- 🟡 4 files modified:
src/sentry/incidents/grouptype.py,src/sentry/workflow_engine/models/detector.py,src/sentry/workflow_engine/processors/detector.py,tests/sentry/workflow_engine/processors/test_detector.py
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 4 files"/]
f1["🟡 grouptype.py +3/-8"]
pr_0 --> f1
f2["🟡 detector.py +7/-1"]
pr_0 --> f2
f3["🟡 detector.py +22/-23"]
pr_0 --> f3
f4["🟡 test_detector.py +217/-119"]
pr_0 --> f4
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 3 |
| 🟡 Minor | 3 |
| 🔵 Info | 2 |
Review details
- Reviewed at: 2026-04-08T00:51:15.441Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 73.6s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| ) -> list[DetectorEvaluationResult]: | ||
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): |
There was a problem hiding this comment.
🔴 CRITICAL — API contract violation (confidence: 100%)
MetricAlertDetectorHandler now inherits from StatefulDetectorHandler but does not implement the required abstract method build_occurrence_and_event_data. Since it's declared as a concrete class (not abstract), attempting to instantiate it will raise a TypeError at runtime. This is a production blocker for any code path that creates a MetricAlertDetectorHandler instance.
Evidence:
- StatefulDetectorHandler declares
build_occurrence_and_event_dataas @abc.abstractmethod in detector.py lines 166-171 - StatefulDetectorHandler also has abstract methods
get_dedupe_valueandget_group_key_valuesthat are not implemented - MetricAlertDetectorHandler is
passonly — no method implementations - The old MetricAlertDetectorHandler had a concrete
evaluate()that returned[], so it could be instantiated; the new one cannot - Detector.detector_handler property instantiates the handler class, so any detector with this type will fail
Agent: architecture
| @@ -45,7 +46,7 @@ class DetectorEvaluationResult: | |||
|
|
|||
| def process_detectors( | |||
| data_packet: DataPacket, detectors: list[Detector] | |||
There was a problem hiding this comment.
🟠 MAJOR — Cross-service impact / API contract violation (confidence: 99%)
The return type of process_detectors() changed from list[tuple[Detector, list[DetectorEvaluationResult]]] to list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]. Any downstream consumer that indexes, iterates, or unpacks the second tuple element as a list (e.g., for result in detector_results, detector_results[0], len(detector_results)) will behave differently. While the blast radius analysis shows 0 affected functions, this is a public API of the module and callers outside the analyzed scope could break.
Evidence:
- Return type annotation changed on line 48
- The function is imported and used in test_detector.py — tests were updated, but other callers in the codebase may not have been
- Dict iteration yields keys by default, not values — any
for result in detector_resultspattern on the second tuple element would silently iterate over keys instead of DetectorEvaluationResult objects
Agent: architecture
| result, event_data = self.build_occurrence_and_event_data( | ||
| group_key, value, PriorityLevel(new_status) | ||
| ) | ||
| return DetectorEvaluationResult( |
There was a problem hiding this comment.
🟠 MAJOR — Module boundaries / Type safety (confidence: 94%)
The code converts new_status (which is DetectorPriorityLevel) to PriorityLevel via PriorityLevel(new_status) on line 301. This works only because DetectorPriorityLevel integer values happen to match PriorityLevel values. However, DetectorPriorityLevel.OK = 0 while PriorityLevel does not have a 0 value — if new_status == DetectorPriorityLevel.OK, PriorityLevel(0) would raise a ValueError. The else branch (line 299) means OK is excluded here, but this implicit coupling between two separate enum types is fragile and any future refactoring that changes the branching could cause a runtime error.
Evidence:
- DetectorPriorityLevel is defined with OK=0 in workflow_engine/types.py
- PriorityLevel values are typically 25 (LOW), 50 (MEDIUM), 75 (HIGH), 100 (CRITICAL) — no 0 value
- Line 301:
PriorityLevel(new_status)performs an implicit int-based conversion between different enum types - The intent specification explicitly flags: 'PriorityLevel vs DetectorPriorityLevel — both exist in the codebase; confirm that the new_status parameter and internal state comparisons use consistent types without implicit conversion errors'
Agent: architecture
| results.append(result) | ||
| results[result.group_key] = result | ||
| return results | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Pattern violation / Silent data loss (confidence: 84%)
The dict-based return from evaluate() uses result.group_key as the key. If get_group_key_values() returns multiple entries that map to the same group_key after evaluation (or if evaluate_group_key_value returns results with identical group_keys), later entries silently overwrite earlier ones. The old code had explicit duplicate detection with logging. The new code silently loses data. While the intent specification says 'dict structure inherently prevents duplicates', the old logging existed because duplicates were observed in practice.
Evidence:
- Line 244:
results[result.group_key] = result— dict assignment overwrites without warning - The removed code (old lines 62-72) explicitly logged 'Duplicate detector state group keys found' as an error
- The old test
test_state_results_multi_group_dupewas removed rather than replaced with a dict-semantics equivalent - group_key comes from
get_group_key_values()keys, which are also used as dict keys inall_state_data, so true duplicates from that source are impossible — but a subclass could override evaluate_group_key_value to return a different group_key than expected
Agent: architecture
| @@ -1,5 +1,6 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| import builtins | |||
There was a problem hiding this comment.
🟡 MINOR — Dependency drift (confidence: 93%)
import builtins is used solely to annotate builtins.type[GroupType] to avoid shadowing with a local name. This is unconventional and may confuse other developers. The standard Python pattern is to use Type from typing (pre-3.9) or type directly in annotations with from __future__ import annotations.
Evidence:
- Line 1 already has
from __future__ import annotations, which makes all annotations strings and avoids the name resolution issue - With
from __future__ import annotations, the return typetype[GroupType] | Nonewould work without needingbuiltins.type - The
builtinsimport adds an unusual dependency that doesn't follow existing codebase patterns
Agent: architecture
| return 1 | ||
|
|
||
| @property | ||
| def group_type(self) -> builtins.type[GroupType] | None: |
There was a problem hiding this comment.
🟡 MINOR — Scalability concerns (confidence: 93%)
The new group_type property performs a registry lookup on every access. The detector_handler property now calls self.group_type which means two separate call sites could invoke the registry lookup multiple times per request/evaluation cycle. If this property is accessed in hot paths, consider caching.
Evidence:
- Line 58-59:
group_typeproperty callsgrouptype.registry.get_by_slug(self.type)each time - Line 63:
detector_handlercallsself.group_type, triggering the lookup - The intent specification notes: 'if the registry lookup is expensive or has side effects, refactoring to a cached property may be needed'
Agent: architecture
| @@ -9,6 +10,7 @@ | |||
| from sentry.backup.scopes import RelocationScope | |||
| from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model | |||
| from sentry.issues import grouptype | |||
There was a problem hiding this comment.
🟡 MINOR — naming conventions (confidence: 83%)
Import of builtins module is unconventional. The builtins.type annotation should use the standard type directly or import from typing.
Evidence:
- Line 12 imports
builtinsbut it's used only once on line 57 asbuiltins.type[GroupType] - Standard Python practice is to use
type[GroupType]directly without thebuiltins.prefix - This pattern is not seen elsewhere in the codebase and adds unnecessary verbosity
Agent: style
| self, data_packet: DataPacket[T] | ||
| ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: | ||
| """ | ||
| Evaluates a given data packet and returns a list of `DetectorEvaluationResult`. |
There was a problem hiding this comment.
🔵 INFO — code patterns (confidence: 84%)
The evaluate() method docstring references returning 'a list' but the method now returns a dict. The docstring is outdated.
Evidence:
- Line 226 docstring states: 'Evaluates a given data packet and returns a list of
DetectorEvaluationResult.' - The actual return type on line 228 is
dict[DetectorGroupKey, DetectorEvaluationResult] - The rest of the docstring also assumes list behavior and needs updates
Agent: style
| from sentry.workflow_engine.models import DataPacket | ||
| from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler | ||
| from sentry.workflow_engine.processors.detector import StatefulDetectorHandler | ||
|
|
There was a problem hiding this comment.
🔵 INFO — documentation (confidence: 84%)
The MetricAlertDetectorHandler class body is now empty (pass), but the TODO comment above it says 'This will be a stateful detector when we build that abstraction.' This comment is now outdated since the class IS now a stateful detector.
Evidence:
- Line 7 has a TODO comment that is no longer applicable
- The class now inherits from
StatefulDetectorHandlerinstead ofDetectorHandler - The comment suggests this was a future refactoring, which has now been completed
Agent: style
| ) -> list[DetectorEvaluationResult]: | ||
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (20 lines, 1 file) — review recommended)
MetricAlertDetectorHandler now inherits from StatefulDetectorHandler but does not implement the required abstract method
build_occurrence_and_event_data. Since it's declared as a concrete class (not abstract), attempting to instantiate it will raise a TypeError at runtime. This is a production blocker for any code path that creates a MetricAlertDetectorHandler instance.
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -1,12 +1,14 @@
from dataclasses import dataclass
+from typing import Any
from sentry.incidents.utils.types import QuerySubscriptionUpdate
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
-from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
+from sentry.workflow_engine.models import DataPacket
+from sentry.workflow_engine.processors.detector import DetectorGroupKey, StatefulDetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+ def build_occurrence_and_event_data(
+ self, group_key: DetectorGroupKey, new_status: Any, is_new_group: bool
+ ) -> Any:
+ # TODO: Implement — produce IssueOccurrence + event data for metric alert fires
+ return None
+
+ def get_dedupe_value(self, data_packet: DataPacket[QuerySubscriptionUpdate]) -> int:
+ # TODO: Implement — return a deduplication value derived from the subscription update
+ return 0
+
+ def get_group_key_values(
+ self, data_packet: DataPacket[QuerySubscriptionUpdate]
+ ) -> dict[DetectorGroupKey, int]:
+ # TODO: Implement — return keyed numeric values driving detector state transitions
+ return {}🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @@ -45,7 +46,7 @@ class DetectorEvaluationResult: | |||
|
|
|||
| def process_detectors( | |||
| data_packet: DataPacket, detectors: list[Detector] | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (28 lines, 1 file) — review recommended)
The return type of
process_detectors()changed fromlist[tuple[Detector, list[DetectorEvaluationResult]]]tolist[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]. Any downstream consumer that indexes, iterates, or unpacks the second tuple element as a list (e.g.,for result in detector_results,detector_results[0],len(detector_results)) will behave differently. While the blast radius analysis shows 0 affected functions, this is a public API of the module and callers outside the analyzed scope could break.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -46,7 +46,14 @@ class DetectorEvaluationResult:
def process_detectors(
data_packet: DataPacket, detectors: list[Detector]
) -> list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]:
+ """
+ Evaluates each detector against the given data packet.
+
+ Returns a list of (Detector, results) tuples where results is a dict
+ keyed by DetectorGroupKey. NOTE: Callers must iterate over
+ ``results.values()`` to access DetectorEvaluationResult objects —
+ iterating over the dict directly yields keys, not results.
+ """
results = []
for detector in detectors:
@@ -58,6 +65,16 @@ def process_detectors(
detector_results = handler.evaluate(data_packet)
for result in detector_results.values():
+ if result.group_key in detector_results and list(detector_results.keys()).count(result.group_key) > 1:
+ # This shouldn't happen - log an error and continue on, but we should investigate this.
+ logger.error(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": detector.id,
+ "group_key": result.group_key,
+ },
+ )
+ continue
+
if result.result is not None:
create_issue_occurrence_from_result(result)
@@ -234,10 +251,16 @@ def evaluate(
dedupe_value = self.get_dedupe_value(data_packet)
group_values = self.get_group_key_values(data_packet)
all_state_data = self.get_state_data(list(group_values.keys()))
results = {}
for group_key, group_value in group_values.items():
result = self.evaluate_group_key_value(
group_key, group_value, all_state_data[group_key], dedupe_value
)
if result:
+ if result.group_key in results:
+ logger.error(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": result.group_key,
+ },
+ )
+ continue
results[result.group_key] = result
return results🤖 Grapple PR auto-fix • major • Review this diff before applying
| result, event_data = self.build_occurrence_and_event_data( | ||
| group_key, value, PriorityLevel(new_status) | ||
| ) | ||
| return DetectorEvaluationResult( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (36 lines, 1 file) — review recommended)
The code converts
new_status(which isDetectorPriorityLevel) toPriorityLevelviaPriorityLevel(new_status)on line 301. This works only becauseDetectorPriorityLevelinteger values happen to matchPriorityLevelvalues. However,DetectorPriorityLevel.OK = 0whilePriorityLeveldoes not have a 0 value — ifnew_status == DetectorPriorityLevel.OK,PriorityLevel(0)would raise a ValueError. Theelsebranch (line 299) means OK is excluded here, but this implicit coupling between two separate enum types is fragile and any future refactoring that changes the branching could cause a runtime error.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import abc
+import dataclasses
import logging
from datetime import timedelta
from typing import Any
@@ -14,6 +15,7 @@
from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka
from sentry.issues.status_change_message import StatusChangeMessage
from sentry.models.group import GroupStatus
+from sentry.types.group import PriorityLevel
from sentry.utils import metrics, redis
from sentry.utils.function_cache import cache_func_for_models
from sentry.utils.iterators import chunked
@@ -20,6 +22,28 @@
from sentry.workflow_engine.models import DataPacket, Detector
from sentry.workflow_engine.types import DetectorGroupKey, DetectorPriorityLevel
+# Explicit mapping from DetectorPriorityLevel to PriorityLevel.
+#
+# IMPORTANT: Do NOT replace this with PriorityLevel(new_status) — an implicit
+# integer cast. DetectorPriorityLevel.OK == 0 has no corresponding PriorityLevel
+# value and would raise a ValueError. Even for non-OK values the integer
+# coincidence is accidental; this mapping makes the relationship auditable and
+# will surface a clear error if the enums ever diverge.
+_DETECTOR_PRIORITY_TO_PRIORITY_LEVEL: dict[DetectorPriorityLevel, PriorityLevel] = {
+ DetectorPriorityLevel.LOW: PriorityLevel.LOW,
+ DetectorPriorityLevel.MEDIUM: PriorityLevel.MEDIUM,
+ DetectorPriorityLevel.HIGH: PriorityLevel.HIGH,
+ DetectorPriorityLevel.CRITICAL: PriorityLevel.CRITICAL,
+ # DetectorPriorityLevel.OK is intentionally excluded: callers must handle
+ # the OK case before converting (it maps to GroupStatus.RESOLVED, not a
+ # PriorityLevel).
+}
+
+
+def detector_priority_to_priority_level(level: DetectorPriorityLevel) -> PriorityLevel:
+ """Convert a non-OK DetectorPriorityLevel to a PriorityLevel."""
+ try:
+ return _DETECTOR_PRIORITY_TO_PRIORITY_LEVEL[level]
+ except KeyError:
+ raise ValueError(
+ f"Cannot convert DetectorPriorityLevel.{level.name} to PriorityLevel. "
+ f"Only non-OK levels have a PriorityLevel equivalent."
+ )
+
+
logger = logging.getLogger(__name__)
@@ -286,9 +310,12 @@ def evaluate_group_key_value(
self.enqueue_state_update(group_key, is_active, new_status)
event_data = None
result: StatusChangeMessage | IssueOccurrence
if new_status == DetectorPriorityLevel.OK:
# If we've determined that we're now ok, we just want to resolve the issue
result = StatusChangeMessage(
fingerprint=self.build_fingerprint(group_key),
project_id=self.detector.project_id,
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)
else:
+ # Use the explicit mapping function rather than PriorityLevel(new_status).
+ # The implicit integer cast would raise ValueError for OK=0 and is
+ # fragile if either enum's values change in future.
result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
+ group_key, value, detector_priority_to_priority_level(new_status)
)
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,
priority=new_status,
result=result,
event_data=event_data,
)
return None🤖 Grapple PR auto-fix • major • Review this diff before applying
| results.append(result) | ||
| results[result.group_key] = result | ||
| return results | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (27 lines, 1 file) — review recommended)
The dict-based return from
evaluate()usesresult.group_keyas the key. Ifget_group_key_values()returns multiple entries that map to the same group_key after evaluation (or ifevaluate_group_key_valuereturns results with identical group_keys), later entries silently overwrite earlier ones. The old code had explicit duplicate detection with logging. The new code silently loses data. While the intent specification says 'dict structure inherently prevents duplicates', the old logging existed because duplicates were observed in practice.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -234,11 +234,27 @@ def evaluate(
dedupe_value = self.get_dedupe_value(data_packet)
group_values = self.get_group_key_values(data_packet)
all_state_data = self.get_state_data(list(group_values.keys()))
results = {}
for group_key, group_value in group_values.items():
result = self.evaluate_group_key_value(
group_key, group_value, all_state_data[group_key], dedupe_value
)
if result:
- results[result.group_key] = result
+ if result.group_key != group_key:
+ # A subclass returned a result whose group_key differs from the
+ # iteration key. This indicates a bug in the override — log an
+ # error and use the iteration key so we don't silently corrupt
+ # the results dict with an unexpected key.
+ logger.error(
+ "Detector evaluate_group_key_value returned mismatched group_key",
+ extra={
+ "detector_id": self.detector.id,
+ "expected_group_key": group_key,
+ "returned_group_key": result.group_key,
+ },
+ )
+ if result.group_key in results:
+ # This should not happen — log an error to match the behaviour
+ # of the old list-based duplicate detection (see removed lines
+ # that logged "Duplicate detector state group keys found").
+ logger.error(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": result.group_key,
+ },
+ )
+ continue
+ results[result.group_key] = result
return results🤖 Grapple PR auto-fix • major • Review this diff before applying
| @@ -1,5 +1,6 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| import builtins | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (3 lines, 1 file))
import builtinsis used solely to annotatebuiltins.type[GroupType]to avoid shadowing with a local name. This is unconventional and may confuse other developers. The standard Python pattern is to useTypefromtyping(pre-3.9) ortypedirectly in annotations withfrom __future__ import annotations.
--- a/src/sentry/workflow_engine/models/detector.py
+++ b/src/sentry/workflow_engine/models/detector.py
@@ -1,7 +1,6 @@
from __future__ import annotations
-import builtins
import logging
from typing import TYPE_CHECKING
@@ -55,7 +54,7 @@ def project_id(self):
return 1
@property
- def group_type(self) -> builtins.type[GroupType] | None:
+ def group_type(self) -> type[GroupType] | None:
return grouptype.registry.get_by_slug(self.type)
@property🤖 Grapple PR auto-fix • minor • Review this diff before applying
| return 1 | ||
|
|
||
| @property | ||
| def group_type(self) -> builtins.type[GroupType] | None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (4 lines, 1 file))
The new
group_typeproperty performs a registry lookup on every access. Thedetector_handlerproperty now callsself.group_typewhich means two separate call sites could invoke the registry lookup multiple times per request/evaluation cycle. If this property is accessed in hot paths, consider caching.
--- a/src/sentry/workflow_engine/models/detector.py
+++ b/src/sentry/workflow_engine/models/detector.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import builtins
+from functools import cached_property
import logging
from typing import TYPE_CHECKING
@@ -53,7 +54,7 @@ def project_id(self):
# XXX: Temporary property until we add `project_id` to the model.
return 1
- @property
+ @cached_property
def group_type(self) -> builtins.type[GroupType] | None:
+ # Registry lookup is memoized per instance to avoid repeated lookups in hot paths
return grouptype.registry.get_by_slug(self.type)
@property🤖 Grapple PR auto-fix • minor • Review this diff before applying
| @@ -9,6 +10,7 @@ | |||
| from sentry.backup.scopes import RelocationScope | |||
| from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model | |||
| from sentry.issues import grouptype | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Small fix (3 lines, 1 file))
Import of
builtinsmodule is unconventional. Thebuiltins.typeannotation should use the standardtypedirectly or import fromtyping.
--- a/src/sentry/workflow_engine/models/detector.py
+++ b/src/sentry/workflow_engine/models/detector.py
@@ -1,7 +1,6 @@
from __future__ import annotations
-import builtins
import logging
from typing import TYPE_CHECKING
@@ -55,7 +55,7 @@ def project_id(self):
return 1
@property
- def group_type(self) -> builtins.type[GroupType] | None:
+ def group_type(self) -> type[GroupType] | None:
return grouptype.registry.get_by_slug(self.type)
@property🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR refactors the stateful detector evaluation pipeline to use a dict-keyed result structure instead of a list, and adds an abstract build_occurrence_and_event_data hook that subclasses must implement to produce IssueOccurrences from detector state transitions.
Changes: 4 files (+249 / -151 lines)
- 🟡 4 files modified:
src/sentry/incidents/grouptype.py,src/sentry/workflow_engine/models/detector.py,src/sentry/workflow_engine/processors/detector.py,tests/sentry/workflow_engine/processors/test_detector.py
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 4 files"/]
f1["🟡 grouptype.py +3/-8"]
pr_0 --> f1
f2["🟡 detector.py +7/-1"]
pr_0 --> f2
f3["🟡 detector.py +22/-23"]
pr_0 --> f3
f4["🟡 test_detector.py +217/-119"]
pr_0 --> f4
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 3 |
| 🟡 Minor | 3 |
| 🔵 Info | 1 |
Review details
- Reviewed at: 2026-04-12T03:15:53.237Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 70.0s
- Low-confidence findings suppressed: 3
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| from sentry.workflow_engine.processors.detector import StatefulDetectorHandler | ||
|
|
||
|
|
||
| # TODO: This will be a stateful detector when we build that abstraction |
There was a problem hiding this comment.
🔴 CRITICAL — Business logic correctness (confidence: 100%)
MetricAlertDetectorHandler now extends StatefulDetectorHandler which has an abstract method build_occurrence_and_event_data. Since MetricAlertDetectorHandler only has pass as its body, it does not implement this abstract method. Any attempt to instantiate MetricAlertDetectorHandler will raise TypeError at runtime. While the intent specification acknowledges this as 'acceptable as a temporary state', any existing code path that instantiates MetricAlertDetectorHandler (e.g., via Detector.detector_handler property with a MetricAlertFire type detector) will break immediately.
Evidence:
- MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): pass
- StatefulDetectorHandler defines @abc.abstractmethod build_occurrence_and_event_data
- StatefulDetectorHandler also defines @abc.abstractmethod get_dedupe_value and get_group_key_values which are also not implemented
- ABC prevents instantiation of classes with unimplemented abstract methods
Agent: logic
| ) | ||
| if result: | ||
| results.append(result) | ||
| results[result.group_key] = result |
There was a problem hiding this comment.
🟠 MAJOR — Edge case - silent duplicate suppression (confidence: 100%)
The evaluate() method iterates over group_values.items() and stores results in a dict keyed by result.group_key. If get_group_key_values returns keys that map to different values but evaluate_group_key_value produces results with the same group_key (e.g., via some transformation), the dict will silently keep only the last result. The previously explicit duplicate-key error log has been removed, so this scenario now has zero observability. While the intent notes this is by design, the removal of the error log means any future bug in get_group_key_values that produces duplicate keys will be completely silent.
Evidence:
- Old code had explicit duplicate detection with logger.error('Duplicate detector state group keys found')
- New code uses dict assignment: results[result.group_key] = result (last-write-wins)
- Intent specification flags this: 'Silent removal of duplicate group key error logging in process_detectors'
Agent: logic
| else: | ||
| result, event_data = self.build_occurrence_and_event_data( | ||
| group_key, value, PriorityLevel(new_status) | ||
| ) |
There was a problem hiding this comment.
🟠 MAJOR — Data flow - enum type mismatch (confidence: 99%)
build_occurrence_and_event_data is called with PriorityLevel(new_status) where new_status is a DetectorPriorityLevel enum value. This relies on implicit integer conversion between DetectorPriorityLevel and PriorityLevel enums. If these enums have different integer values for the same conceptual priority levels, this conversion will produce incorrect results or raise ValueError. The test mock (build_mock_occurrence_and_event) accepts PriorityLevel and uses new_status.value for initial_issue_priority, so the conversion must be correct for all priority levels.
Evidence:
- DetectorPriorityLevel is defined in sentry.workflow_engine.types
- PriorityLevel is defined in sentry.types.group
- Line 301: PriorityLevel(new_status) - this converts DetectorPriorityLevel enum to PriorityLevel via integer value
- If the integer mappings differ between these two enums, this will silently produce wrong priorities or raise ValueError
Agent: logic
| event_data, | ||
| ) | ||
| assert results == [ | ||
| ( |
There was a problem hiding this comment.
🟠 MAJOR — Edge case - test assertion uses wrong value (confidence: 99%)
In test_state_results_multi_group, the second occurrence is built with build_mock_occurrence_and_event(handler, 'group_2', 6, PriorityLevel.HIGH) passing value=6, but the data packet has 'group_2': 10. The actual handler's build_occurrence_and_event_data will be called with value=10 (the real value from the data packet), not 6. This means the test assertion may be comparing against an incorrect expected occurrence if value is used in the occurrence data.
Evidence:
- DataPacket('1', {'dedupe': 2, 'group_vals': {'group_1': 6, 'group_2': 10}})
- build_mock_occurrence_and_event(detector.detector_handler, 'group_2', 6, PriorityLevel.HIGH) - passes value=6
- The actual evaluate() call will pass value=10 for group_2 to build_occurrence_and_event_data
- However, looking at build_mock_occurrence_and_event, the
valueparameter is received but never actually used in the occurrence construction, so this bug is currently masked
Agent: logic
| results.append(result) | ||
| results[result.group_key] = result | ||
| return results | ||
|
|
There was a problem hiding this comment.
🟡 MINOR — Documentation (confidence: 100%)
The docstring for StatefulDetectorHandler.evaluate() still refers to returning 'a list of DetectorEvaluationResult' but the method now returns a dict. The docstring should be updated to reflect the new return type.
Evidence:
- Line 230: 'Evaluates a given data packet and returns a list of
DetectorEvaluationResult.' - Return type on line 227: -> dict[DetectorGroupKey, DetectorEvaluationResult]
- Docstring at line 226-228 says: 'Evaluates a given data packet and returns a list of
DetectorEvaluationResult.' - Actual return type changed to dict[DetectorGroupKey, DetectorEvaluationResult] at line 227-229
- This creates a mismatch between documentation and implementation that could confuse maintainers
Agent: style
| group_type = self.group_type | ||
| if not group_type: | ||
| logger.error( | ||
| "No registered grouptype for detector", |
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work (confidence: 95%)
Detector.group_type introduces a second registry lookup when detector_handler is called, since detector_handler now calls self.group_type instead of a local variable. Previously there was one registry lookup per detector_handler access; now any caller that uses both group_type and detector_handler will hit the registry twice. The registry lookup (get_by_slug) likely involves a dict scan or similar O(n) operation over registered group types.
Evidence:
- group_type property calls grouptype.registry.get_by_slug(self.type) unconditionally on every access
- detector_handler property now calls self.group_type, meaning each call to detector_handler triggers a registry lookup via the property
- In tests, detector.detector_handler and handler.detector.group_type are both called in build_mock_occurrence_and_event, causing two registry lookups per invocation
- The risk area in the PR description itself notes: 'if detector_handler and group_type are both called, the registry is hit twice'
Agent: performance
| return build_mock_occurrence_and_event(self, group_key, value, new_status) | ||
|
|
||
|
|
||
| class BaseDetectorHandlerTest(BaseGroupTypeTest): |
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 84%)
MockDetectorStateHandler class defined at the top of the test file before the BaseDetectorHandlerTest class is created, but it depends on a helper function build_mock_occurrence_and_event that is defined much later in the file (line 254). This violates typical Python convention of defining dependencies before use.
Evidence:
- MockDetectorStateHandler is defined at line 26-42
- It calls build_mock_occurrence_and_event at line 35
- build_mock_occurrence_and_event is defined at line 254, much later in the file
- While this works due to Python's runtime behavior, it's poor code organization
Agent: style
| results.append(result) | ||
| results[result.group_key] = result | ||
| return results | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (2 lines, 1 file))
The docstring for StatefulDetectorHandler.evaluate() still refers to returning 'a list of DetectorEvaluationResult' but the method now returns a dict. The docstring should be updated to reflect the new return type.
| Evaluates a given data packet and returns a dict of `DetectorEvaluationResult` keyed by `DetectorGroupKey`. |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| group_type = self.group_type | ||
| if not group_type: | ||
| logger.error( | ||
| "No registered grouptype for detector", |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (2 lines, 1 file))
Detector.group_type introduces a second registry lookup when detector_handler is called, since detector_handler now calls self.group_type instead of a local variable. Previously there was one registry lookup per detector_handler access; now any caller that uses both group_type and detector_handler will hit the registry twice. The registry lookup (get_by_slug) likely involves a dict scan or similar O(n) operation over registered group types.
| "No registered grouptype for detector", | |
| group_type = grouptype.registry.get_by_slug(self.type) |
🤖 Grapple PR auto-fix • minor • confidence: 95%
| from sentry.workflow_engine.processors.detector import StatefulDetectorHandler | ||
|
|
||
|
|
||
| # TODO: This will be a stateful detector when we build that abstraction |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (13 lines, 1 file) — review recommended)
MetricAlertDetectorHandler now extends StatefulDetectorHandler which has an abstract method
build_occurrence_and_event_data. Since MetricAlertDetectorHandler only haspassas its body, it does not implement this abstract method. Any attempt to instantiate MetricAlertDetectorHandler will raise TypeError at runtime. While the intent specification acknowledges this as 'acceptable as a temporary state', any existing code path that instantiates MetricAlertDetectorHandler (e.g., via Detector.detector_handler property with a MetricAlertFire type detector) will break immediately.
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -4,10 +4,11 @@
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
-from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
+from sentry.workflow_engine.models import DataPacket
+from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
-class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]):
+ def evaluate(
+ self, data_packet: DataPacket[QuerySubscriptionUpdate]
+ ) -> list[DetectorEvaluationResult]:
+ # TODO: Implement - will migrate to StatefulDetectorHandler once
+ # build_occurrence_and_event_data, get_dedupe_value, and get_group_key_values
+ # are implemented for metric alert detectors
+ return []🤖 Grapple PR auto-fix • critical • Review this diff before applying
| ) | ||
| if result: | ||
| results.append(result) | ||
| results[result.group_key] = result |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (16 lines, 1 file) — review recommended)
The evaluate() method iterates over group_values.items() and stores results in a dict keyed by result.group_key. If get_group_key_values returns keys that map to different values but evaluate_group_key_value produces results with the same group_key (e.g., via some transformation), the dict will silently keep only the last result. The previously explicit duplicate-key error log has been removed, so this scenario now has zero observability. While the intent notes this is by design, the removal of the error log means any future bug in get_group_key_values that produces duplicate keys will be completely silent.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -234,10 +234,20 @@ def evaluate(
dedupe_value = self.get_dedupe_value(data_packet)
group_values = self.get_group_key_values(data_packet)
all_state_data = self.get_state_data(list(group_values.keys()))
results = {}
for group_key, group_value in group_values.items():
result = self.evaluate_group_key_value(
group_key, group_value, all_state_data[group_key], dedupe_value
)
if result:
+ if result.group_key in results:
+ # This can happen if get_group_key_values returns keys that
+ # map to the same result.group_key after transformation in
+ # evaluate_group_key_value. Last-write-wins is intentional
+ # per the dict-keyed design, but log a warning so any future
+ # bug in get_group_key_values producing duplicate keys is
+ # observable. See: Silent removal of duplicate group key
+ # error logging in process_detectors.
+ logger.warning(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": result.group_key,
+ "input_group_key": group_key,
+ },
+ )
results[result.group_key] = result
return results🤖 Grapple PR auto-fix • major • Review this diff before applying
| else: | ||
| result, event_data = self.build_occurrence_and_event_data( | ||
| group_key, value, PriorityLevel(new_status) | ||
| ) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (19 lines, 1 file) — review recommended)
build_occurrence_and_event_data is called with
PriorityLevel(new_status)wherenew_statusis aDetectorPriorityLevelenum value. This relies on implicit integer conversion betweenDetectorPriorityLevelandPriorityLevelenums. If these enums have different integer values for the same conceptual priority levels, this conversion will produce incorrect results or raise ValueError. The test mock (build_mock_occurrence_and_event) accepts PriorityLevel and usesnew_status.valueforinitial_issue_priority, so the conversion must be correct for all priority levels.
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -14,6 +14,7 @@
from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka
from sentry.issues.status_change_message import StatusChangeMessage
from sentry.models.group import GroupStatus
+from sentry.types.group import PriorityLevel
from sentry.utils import metrics, redis
from sentry.utils.function_cache import cache_func_for_models
from sentry.utils.iterators import chunked
@@ -20,6 +21,14 @@ from sentry.workflow_engine.types import (
DetectorGroupKey,
DetectorPriorityLevel,
)
+
+# Explicit mapping from DetectorPriorityLevel to PriorityLevel.
+# Do NOT replace this with PriorityLevel(detector_priority_level) — the two enums
+# may not share identical integer values, and implicit coercion would silently
+# assign the wrong priority if they ever diverge.
+DETECTOR_PRIORITY_TO_ISSUE_PRIORITY: dict[DetectorPriorityLevel, PriorityLevel] = {
+ DetectorPriorityLevel.LOW: PriorityLevel.LOW,
+ DetectorPriorityLevel.MEDIUM: PriorityLevel.MEDIUM,
+ DetectorPriorityLevel.HIGH: PriorityLevel.HIGH,
+ DetectorPriorityLevel.CRITICAL: PriorityLevel.CRITICAL,
+}
@@ -295,7 +304,12 @@ def evaluate_group_key_value(
new_substatus=None,
)
else:
+ # Use the explicit mapping — never rely on int coercion between
+ # DetectorPriorityLevel and PriorityLevel, as their integer values
+ # are not guaranteed to be identical. A KeyError here means a new
+ # DetectorPriorityLevel was added without updating the mapping above.
+ issue_priority = DETECTOR_PRIORITY_TO_ISSUE_PRIORITY[new_status]
result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
+ group_key, value, issue_priority
)🤖 Grapple PR auto-fix • major • Review this diff before applying
| event_data, | ||
| ) | ||
| assert results == [ | ||
| ( |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (6 lines, 1 file))
In test_state_results_multi_group, the second occurrence is built with
build_mock_occurrence_and_event(handler, 'group_2', 6, PriorityLevel.HIGH)passing value=6, but the data packet has'group_2': 10. The actual handler's build_occurrence_and_event_data will be called with value=10 (the real value from the data packet), not 6. This means the test assertion may be comparing against an incorrect expected occurrence if value is used in the occurrence data.
--- a/tests/sentry/workflow_engine/processors/test_detector.py
+++ b/tests/sentry/workflow_engine/processors/test_detector.py
@@ -200,7 +200,7 @@ def test_state_results_multi_group(self, mock_produce_occurrence_to_kafka):
occurrence_2, event_data_2 = build_mock_occurrence_and_event(
- detector.detector_handler, "group_2", 6, PriorityLevel.HIGH
+ detector.detector_handler, "group_2", 10, PriorityLevel.HIGH
)
@@ -486,7 +486,7 @@ def test_results_on_change(self):
occurrence, event_data = build_mock_occurrence_and_event(
- handler, "val1", 6, PriorityLevel.HIGH
+ handler, "val1", 100, PriorityLevel.HIGH
)
@@ -519,7 +519,7 @@ def test_dedupe(self):
occurrence, event_data = build_mock_occurrence_and_event(
- handler, "val1", 6, PriorityLevel.HIGH
+ handler, "val1", 8, PriorityLevel.HIGH
)🤖 Grapple PR auto-fix • major • Review this diff before applying
2 participants
Benchmark test PR ai-code-review-evaluation#10 - Replicating Greptile benchmark for Grapple PR evaluation.