Conversation
…fications for metric issues
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| ): | ||
| return False | ||
|
|
||
| return features.has("organizations:workflow-engine-metric-issue-ui", organization) |
There was a problem hiding this comment.
Disable flag ignored outside denylist
Medium Severity
organizations:workflow-engine-metric-issue-disable-issue-detector-notifications is only checked when group_type_id is already in workflow_engine.group.type_id.disable_issue_stream_detector. If MetricIssue.type_id is removed from that option, metric issues bypass the new flag and still use the issue stream detector, which undermines the “disable notifications” behavior.
| return {d for d in [self.issue_stream_detector, self.event_detector] if d is not None} | ||
|
|
||
|
|
||
| def _is_issue_stream_detector_enabled(event_data: WorkflowEventData) -> bool: |
There was a problem hiding this comment.
This function makes me uncomfortable.
"how are detectors associated with issues" is core behavior, and in the middle of it, we have this thing that checks a config option, then does a hardcoded id type check, then checks a flag (now two flags).
"Should the issue stream detector be enabled?" is already a somewhat abstract question in terms of product experience (despite being rather impactful) and this config driven logic makes it pretty locally inscrutable too.
I haven't (yet) invested the time to have a productive counterproposal (though I have started working on it and talked to josh about it, broadly I think it looks like phrasing things more directly in product terms, potentially splitting the issue stream use cases up at least conceptually, and putting anything intrinstic on DetectorSettings or similar rather than config, potentially putting the rollout-driven stuff on it's own logical path), so this is mostly just me having opinions.
There was a problem hiding this comment.
Will we have other issue types that need to be excluded once we fully roll out metric issues and remove this targeted disablement flag? Once that is done the issue stream detectors should always be enabled and we can get rid of this function altogether.
4 participants
Closes ISWF-2118