fix(alerts): create and edit issue alert rules via org-scoped /workflows/ - #1223
Conversation
|
Codecov Results 📊✅ Patch coverage is 88.10%. Project has 5324 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.81% 81.86% +0.05%
==========================================
Files 423 423 —
Lines 29343 29356 +13
Branches 19118 19135 +17
==========================================
+ Hits 24006 24032 +26
- Misses 5337 5324 -13
- Partials 1989 1991 +2Generated by Codecov Action |
|
Jared, get this ready to be merged, mark as ready, wait for all bot reviews, address all review comments then merge. |
…ows/
Final slice of migrating issue alerts off the deprecated (HTTP 410
brownout) project-scoped /projects/{org}/{project}/rules/ endpoint
(#1182), after read (#1215) and delete (#1216). This migrates create and
edit.
create/edit now go through the @sentry/api SDK (createOrganizationWorkflow
/ updateOrganizationWorkflow), assembling the workflow-document body:
triggers {logicType, conditions}, actionFilters[{logicType, conditions,
actions}], config.frequency, and detectorIds resolved from the project's
error detector. --action-match/--filter-match map to logic types the same
way the backend dual-write does (any -> any-short, all -> all).
The SDK types conditions/actions as unknown[] and its body fields as
snake_case, but the endpoint's CamelSnakeSerializer wants camelCase, so
the body is built as a record and cast into the typed SDK arg (same
double-cast pattern as getProject). No backend change needed.
BREAKING CHANGE: --condition/--action/--filter now take workflow-native
JSON ({type, comparison, conditionResult} / {type, data, config}) instead
of legacy rule identifiers ({id: 'sentry.rules...'}).
Refs #1182
9bc01f0 to
1a135fb
Compare
|
rebased onto current green locally ( |
![cursor[bot]](https://cdn.github.com/in/1210556?s=60&v=4){kind=small}
The workflows update endpoint returns `enabled` instead of `status`, but `edit` spread the raw response so human output showed `undefined` and JSON dropped `status`. Mirror the create path's enabled->status mapping.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1db256c. Configure here.
|
status on the "address bot reviews then merge" request:
not merging yet: i don't want to auto-merge over an unresolved high-severity correctness question that turns on the author's backend knowledge. @betegon once you confirm whether the workflows create endpoint requires |
The org-scoped workflows create endpoint rejects a trigger group that
carries an issue-alert trigger condition (first_seen_event /
regression_event / reappeared_event / issue_resolved_trigger) unless its
logic type is 'any-short' (BaseDataConditionGroupValidator._validate_logic_type
in getsentry/sentry). We were mapping --action-match all -> logicType 'all'
on the trigger group, so create would fail for the standard issue-alert
triggers. The dual-write in the backend preserves 'all' via .objects.create(),
which bypasses this validator — those groups are explicitly grandfathered,
so the CLI's endpoint path can't rely on that.
Split the mapping: triggerLogicType() always returns 'any-short' for the
trigger ("when") group; matchToLogicType() still maps --filter-match for the
action-filter ("if") group, which never holds trigger conditions and accepts
either logic type. edit does the same so it never writes a non-any-short
trigger. --action-match stays for parity but no longer affects the trigger.
Jared, I'm a bit confused. Does this mean we essentially removed the |
|
No AND capability is lost — the AND that users actually rely on lives on the filter group, which this PR leaves fully intact. Two separate match modes on an issue alert:
So the split is: (One caveat for full honesty: |
|
Jared,
Yeah we should definitely drop it. Is this a sentry-cli v3 compatibility issue that we should note in our docs or just a breaking change with the former version of this command which did not exist in the legacy sentry-cli one? |
Since the trigger group is always 'any-short' for issue alerts, --action-match had no effect on the workflows body — it was accepted (and, on create, required) but silently ignored. Remove it entirely: drop the flag, its -m alias, the required-flag guard on create, and the type/help references. The trigger group's logicType is now pinned to any-short whenever conditions are set. --filter-match (all/any) on the action-filter group — the meaningful AND/OR control — is unchanged. Also refresh the alert docs fragment: the create example dropped --action-match and switched to workflow-native condition/action JSON to match this command's post-migration surface.
|
Done — dropped On the compat question: this is not a sentry-cli v3 / legacy-CLI concern. The legacy Rust |
|
fix-ci: attempt 1 — the failure is the e2e |
|
Shall we also alias |
The -m short was freed up when --action-match was dropped; point it at --filter-match, the remaining match-mode flag. Regenerated skill docs.
|
Done — |
![sentry[bot]](https://cdn.github.com/in/12637?s=60&v=4){kind=small}
| const actionFilters = Array.isArray(body.actionFilters) | ||
| ? (body.actionFilters as Record<string, unknown>[]) | ||
| : []; | ||
| const filter = (actionFilters[0] as Record<string, unknown>) ?? {}; |
There was a problem hiding this comment.
Bug: Partially editing an issue alert can overwrite and destroy the existing filter configuration if the API response for the alert rule is missing the actionFilters array.
Severity: HIGH
Suggested Fix
Before mutating actionFilters, validate that it was successfully loaded from the document returned by getIssueAlertWorkflowDocument. If actionFilters is missing from the API response, ensure the user's partial edits are merged with the existing state rather than creating a new, incomplete object. This will prevent accidental data loss during partial updates.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/cli/src/commands/alert/issues/edit.ts#L123-L126
Potential issue: When a partial edit is applied to an issue alert, such as using only
the `--action` flag, the code may construct a new, incomplete filter object. This occurs
if the response from `getIssueAlertWorkflowDocument` does not include an `actionFilters`
array. The code at lines 123-126 defaults to an empty object (`{}`) in this scenario. A
subsequent PUT request then replaces the entire existing action filter with this
incomplete object, silently removing other configured properties like `logicType` and
`conditions` from the alert rule, effectively destroying the filter configuration.
Did we get this right? 👍 / 👎 to inform future reviews.
BYK
added
jared
|
Jared address the overlooked Seer comment |
2 participants
What & why
Final slice of migrating issue alerts off the deprecated (HTTP 410 brownout) project-scoped
/projects/{org}/{project}/rules/endpoint (#1182), after read (#1215) and delete (#1216). This migrates create and edit.create/editnow go through the@sentry/apiSDK (createOrganizationWorkflow/updateOrganizationWorkflow), assembling the workflow-document body:triggers: { logicType, conditions }from--condition/--action-matchactionFilters: [{ logicType, conditions, actions }]from--filter/--filter-match/--actionconfig.frequencyfrom--frequencydetectorIdsresolved from the project's error detector (an issue alert has to attach to it to fire) vialistOrganizationDetectors(?project=…&query=type:error)--action-match/--filter-matchmap toDataConditionGrouplogic types the same way the backend dual-write does (any→any-short,all→all).editreads the workflow document (getOrganizationWorkflow), applies the flag overrides in workflow shape (--status→enabled), and PUTs it back.The SDK types conditions/actions as
unknown[]and its body fields as snake_case, but the endpoint'sCamelSnakeSerializerwants camelCase — so the body is built as a record and cast into the typed SDK arg, the same double-cast pattern asgetProject. No backend change needed.Breaking change
--condition/--action/--filternow take workflow-native JSON ({type, comparison, conditionResult}/{type, data, config}) instead of the legacy rule identifiers ({id: "sentry.rules..."}). The--helpexamples are updated accordingly.Scope
Read/delete (merged in #1215/#1216) use
apiRequestToRegion; create/edit here use the@sentry/apiSDK + cast, matching the projects-domain SDK adoption (#1213). Minor inconsistency withinalerts.ts, intentional — this is the SDK-adoption direction.Test plan
tsc --noEmit+biome check: clean.vitest run test/lib/api/alerts.test.ts test/commands/alert: green (19 tests). API-level tests pin the exact workflow create/update/detector requests viamockFetch; command tests mock the api-client so they are transparent to the mechanism.config.frequencyunit (minutes, per the CLI flag), (3)query=type:errorreturns the project error detector.Refs #1182