feat(workflow_engine): Add in hook for producing occurrences from the stateful detector

[zaibkhan]

Mirrors ai-code-review-evaluation#10 for like-for-like benchmarking.

• Base: workflow-engine-stateful-detector-before
• Head: workflow-engine-stateful-detector-after

Original PR excerpt:

Test [new PR number opened]

[codoki-pr-intelligence]

Codoki PR Review

Summary: Add occurrence hook, switch results to dict, ensure handler instantiation
What’s good: Solid refactor to dict keyed by group_key simplifies dedupe and improves clarity; new build_occurrence_and_event_data hook integrates occurrence production cleanly; tests assert Kafka production paths.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Correctness — Abstract handler will raise on instantiation	…/incidents/grouptype.py This class now subclasses StatefulDetectorHandler but does not implement required abstract methods (counter_names, get_dedupe_value, get_group_key_values, build_occurrence_and_event_data). Instantiation via Detector.detector_handler will raise TypeError, breaking any Detector using this GroupType.
High	Correctness — Potential KeyError when no counters are defined	…/processors/detector.py Accessing counter_updates[gk] will raise KeyError when counter_names is empty or when no counter values exist for a given group key. This makes StatefulDetectorHandler fragile for detectors that do not track counters.

Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: Harden state retrieval for detectors with no counters to avoid KeyError, and avoid assuming enum value parity when converting DetectorPriorityLevel to PriorityLevel; keep MetricAlertDetectorHandler concrete to prevent runtime instantiation errors.
• Testing: Add coverage for detectors with empty counter_names (ensuring get_state_data returns counter_updates={} per group), and a test asserting that status-change results (DetectorPriorityLevel.OK path) produce a PayloadType.STATUS_CHANGE via create_issue_occurrence_from_result. Consider a unit test for PriorityLevel conversion to guard against mismatched enum values.
• Documentation: Update docstrings: StatefulDetectorHandler.evaluate still states it returns a list, but now returns a dict keyed by DetectorGroupKey. Clarify expected mapping between DetectorPriorityLevel and PriorityLevel or add a helper to document the conversion.
• Compatibility: Changing DetectorHandler.evaluate from list to dict affects any custom handlers; ensure all in-repo implementations are updated. Making MetricAlertDetectorHandler subclass StatefulDetectorHandler without implementing abstract methods will make the GroupType's detector_handler instantiation fail if used.
• Performance: No significant performance concerns; pipeline usage is appropriate. Minor improvement: avoid building/reading Redis counter keys when counter_names is empty and default counter_updates to {} without dict lookups.
• Open questions: Are DetectorPriorityLevel and PriorityLevel guaranteed to have identical numeric values? Do we expect any handler to have an empty counter_names list? Could MetricAlertFire be instantiated by existing detectors in any environment?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Processor as process_detectors
    participant Handler
    participant Kafka as produce_occurrence_to_kafka
    Caller->>Processor: process_detectors(data_packet, detectors)
    loop for each detector
        Processor->>Handler: evaluate(data_packet)
        alt has results
            loop for each result
                alt result.result is IssueOccurrence or StatusChangeMessage
                    Processor->>Kafka: produce_occurrence_to_kafka(payload_type, occurrence/status_change, event_data)
                end
            end
            Processor-->>Caller: append (detector, results)
        end
        Processor->>Handler: commit_state_updates()
    end
    Processor-->>Caller: results

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: This class now subclasses StatefulDetectorHandler but does not implement required abstract methods (counter_names, get_dedupe_value, get_group_key_values, build_occurrence_and_event_data). Instantiation via Detector.detector_handler will raise TypeError, breaking any Detector using this GroupType.

[codoki-pr-intelligence]

🔷 Medium: Casting via PriorityLevel(new_status) assumes enum value parity; if DetectorPriorityLevel values diverge from PriorityLevel, this will raise or map to the wrong level. Using the enum name is safer when names are aligned.

Suggested change

	@@ -298,8 +295,10 @@ def evaluate_group_key_value(
	group_key, value, PriorityLevel[new_status.name]