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
11 changes: 3 additions & 8 deletions src/sentry/incidents/grouptype.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,12 @@
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
from sentry.workflow_engine.models import DataPacket
from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler
from sentry.workflow_engine.processors.detector import StatefulDetectorHandler

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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 StatefulDetectorHandler instead of DetectorHandler
  • The comment suggests this was a future refactoring, which has now been completed

Agent: style


# TODO: This will be a stateful detector when we build that abstraction

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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 pass body
  • StatefulDetectorHandler (detector.py line 166) declares build_occurrence_and_event_data as @abc.abstractmethod
  • No implementation of build_occurrence_and_event_data in MetricAlertDetectorHandler

Agent: style

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [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, 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.

--- 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [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 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.

--- 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

class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]):
def evaluate(
self, data_packet: DataPacket[QuerySubscriptionUpdate]
) -> list[DetectorEvaluationResult]:
# TODO: Implement
return []
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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_data as @abc.abstractmethod in detector.py lines 166-171
  • StatefulDetectorHandler also has abstract methods get_dedupe_value and get_group_key_values that are not implemented
  • MetricAlertDetectorHandler is pass only — 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [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

pass

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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_data as 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [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



# Example GroupType and detector handler for metric alerts. We don't create these issues yet, but we'll use something
Expand Down
8 changes: 7 additions & 1 deletion src/sentry/workflow_engine/models/detector.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import builtins

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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 type type[GroupType] | None would work without needing builtins.type
  • The builtins import adds an unusual dependency that doesn't follow existing codebase patterns

Agent: architecture

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (3 lines, 1 file))

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.

--- 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

import logging
from typing import TYPE_CHECKING

Expand All @@ -9,6 +10,7 @@
from sentry.backup.scopes import RelocationScope

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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 builtins added
  • Line 58: builtins.type[GroupType] | None uses the full qualified name
  • The type builtin does not need to be qualified with builtins.

Agent: style

from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model
from sentry.issues import grouptype

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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 builtins is added and used only in the return annotation builtins.type[GroupType] | None.
  • The standard approach is from typing import Type and use Type[GroupType], or in Python 3.9+ use type[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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 builtins but it's used only once on line 57 as builtins.type[GroupType]
  • Standard Python practice is to use type[GroupType] directly without the builtins. prefix
  • This pattern is not seen elsewhere in the codebase and adds unnecessary verbosity

Agent: style

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — style agent (Small fix (3 lines, 1 file))

Import of builtins module is unconventional. The builtins.type annotation should use the standard type directly or import from typing.

--- 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

from sentry.issues.grouptype import GroupType
from sentry.models.owner_base import OwnerModel

if TYPE_CHECKING:
Expand Down Expand Up @@ -53,9 +55,13 @@ def project_id(self):
# XXX: Temporary property until we add `project_id` to the model.
return 1

@property
def group_type(self) -> builtins.type[GroupType] | None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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_type property calls grouptype.registry.get_by_slug(self.type) each time
  • Line 63: detector_handler calls self.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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (4 lines, 1 file))

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.

--- 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

return grouptype.registry.get_by_slug(self.type)

@property
def detector_handler(self) -> DetectorHandler | None:
group_type = grouptype.registry.get_by_slug(self.type)
group_type = self.group_type
if not group_type:
logger.error(
"No registered grouptype for detector",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [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.

Suggested change
"No registered grouptype for detector",
group_type = grouptype.registry.get_by_slug(self.type)

🤖 Grapple PR auto-fix • minor • confidence: 95%

Expand Down
45 changes: 22 additions & 23 deletions src/sentry/workflow_engine/processors/detector.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -45,7 +46,7 @@ class DetectorEvaluationResult:

def process_detectors(
data_packet: DataPacket, detectors: list[Detector]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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_results pattern on the second tuple element would silently iterate over keys instead of DetectorEvaluationResult objects

Agent: architecture

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (28 lines, 1 file) — review recommended)

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.

--- 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

) -> list[tuple[Detector, list[DetectorEvaluationResult]]]:
) -> list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]:
results = []

for detector in detectors:
Expand All @@ -55,25 +56,11 @@ def process_detectors(
continue

detector_results = handler.evaluate(data_packet)
detector_group_keys = set()

for result in detector_results:
if result.group_key in detector_group_keys:
# 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

for result in detector_results.values():
if result.result is not None:
create_issue_occurrence_from_result(result)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))

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.

Suggested change
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%


detector_group_keys.add(result.group_key)

if detector_results:
results.append((detector, detector_results))

Expand Down Expand Up @@ -136,7 +123,9 @@ def __init__(self, detector: Detector):
self.conditions = []

@abc.abstractmethod
def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]:
def evaluate(
self, data_packet: DataPacket[T]
) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
pass

def commit_state_updates(self):
Expand Down Expand Up @@ -174,6 +163,12 @@ def get_group_key_values(self, data_packet: DataPacket[T]) -> dict[str, int]:
"""
pass

@abc.abstractmethod
def build_occurrence_and_event_data(
self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel
) -> tuple[IssueOccurrence, dict[str, Any]]:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — security agent (Larger fix (30 lines, 1 file) — review recommended)

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.

--- 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

pass

def build_fingerprint(self, group_key) -> list[str]:
"""
Builds a fingerprint to uniquely identify a detected issue
Expand Down Expand Up @@ -228,7 +223,9 @@ def get_state_data(
)
return results

def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]:
def evaluate(
self, data_packet: DataPacket[T]
) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
"""
Evaluates a given data packet and returns a list of `DetectorEvaluationResult`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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

There will be one result for each group key result in the packet, unless the
Expand All @@ -237,13 +234,13 @@ def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]
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 = []
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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

)
if result:
results.append(result)
results[result.group_key] = result

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [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

return results

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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] = result uses the result's group_key, not the iteration variable group_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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [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() 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.

Suggested change
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%


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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_dupe was 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 in all_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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (27 lines, 1 file) — review recommended)

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.

--- 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ [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.

Suggested change
Evaluates a given data packet and returns a dict of `DetectorEvaluationResult` keyed by `DetectorGroupKey`.

🤖 Grapple PR auto-fix • minor • confidence: 100%

def evaluate_group_key_value(
Expand Down Expand Up @@ -289,7 +286,7 @@ 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 = 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(
Expand All @@ -298,8 +295,10 @@ def evaluate_group_key_value(
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)

# TODO: Add hook here for generating occurrence
else:
result, event_data = self.build_occurrence_and_event_data(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Data Flow (confidence: 100%)

The value parameter passed to build_occurrence_and_event_data is the original value variable from the method parameter, not group_value from the condition evaluation. Looking at the method signature evaluate_group_key_value(self, group_key, group_value, state_data, dedupe_value), the parameter is named value at line 265. This is correct but could be confusing since the tests call build_mock_occurrence_and_event with hardcoded value 6 even when the actual group_value is different (e.g., 100 or 200 in test_results_on_change).

Evidence:

  • In test_results_on_change, the data packet has val1: 100 but build_mock_occurrence_and_event is called with value 6
  • The test still passes because build_mock_occurrence_and_event doesn't use the value in fingerprint matching, but the occurrence objects will have different data than what actually triggered them

Agent: logic

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (17 lines, 1 file) — review recommended)

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.

--- 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (12 lines, 1 file) — review recommended)

The value parameter passed to build_occurrence_and_event_data is the original value variable from the method parameter, not group_value from the condition evaluation. Looking at the method signature evaluate_group_key_value(self, group_key, group_value, state_data, dedupe_value), the parameter is named value at line 265. This is correct but could be confusing since the tests call build_mock_occurrence_and_event with hardcoded value 6 even 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

group_key, value, PriorityLevel(new_status)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [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) 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.

--- 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

return DetectorEvaluationResult(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Code organization (confidence: 95%)

Variable result is typed as StatusChangeMessage | IssueOccurrence but the code path for new_status == DetectorPriorityLevel.OK assigns a StatusChangeMessage, while the else branch assigns an IssueOccurrence. The type annotation is imprecise and should reflect the actual assignment. Additionally, event_data is assigned only in the else branch but used unconditionally in the return statement, which could lead to an undefined variable error.

Evidence:

  • Line 289: result: StatusChangeMessage | IssueOccurrence declares union type
  • Line 291-296: Assigns StatusChangeMessage in if branch
  • Line 297-299: Assigns IssueOccurrence in else branch
  • Line 288: event_data = None initialized, then only set in else branch
  • Line 300-308: DetectorEvaluationResult uses both result and event_data without checking if they're defined

Agent: style

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — style agent (Larger fix (12 lines, 1 file) — review recommended)

Variable result is typed as StatusChangeMessage | IssueOccurrence but the code path for new_status == DetectorPriorityLevel.OK assigns a StatusChangeMessage, while the else branch assigns an IssueOccurrence. The type annotation is imprecise and should reflect the actual assignment. Additionally, event_data is 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (36 lines, 1 file) — review recommended)

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.

--- 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

group_key=group_key,
is_active=is_active,
Expand Down
Loading