fix(alertmanager): keep severity=none alerts non-actionable - #1539
Open
allyblockcast[bot] wants to merge 1 commit into
Open
fix(alertmanager): keep severity=none alerts non-actionable#1539allyblockcast[bot] wants to merge 1 commit into
allyblockcast[bot] wants to merge 1 commit into
Conversation
…hored-By: Paperclip <noreply@paperclip.ing>
Author
|
🔗 Paperclip issue: BLO-24177 |
1 similar comment
Author
|
🔗 Paperclip issue: BLO-24177 |
Author
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fd85c3e
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:790— terminal re-fires return before aggregate membership and firing-event reconciliation. When a terminal alert fires, resolves, and later fires again, this branch clearsresolvedAtin state and refreshes the issue, but skipsupsertAggregateMemberandalertmanager.alert.firingemission. The aggregate member can remain marked resolved, so a later resolve may close the shared aggregate issue despite this fingerprint firing, and downstream consumers miss the re-fire.- Run terminal re-fires through the same aggregate fence/member upsert and event/state update path as normal re-fires, or explicitly reset the member and emit the firing event before returning. Add a regression test covering terminal -> resolved -> terminal re-fire and asserting membership is unresolved and the firing event is emitted.
Suggestions (0)
Strengths
- The terminal severity predicate is isolated, normalized case-insensitively, and supports an explicit replacement override.
- Owner and route resolution are avoided for newly created terminal alerts, matching the non-actionable requirement.
- State-loss recovery is covered for an existing terminal issue.
Recommended Action
- Fix the Important issue before merge.
- Add the terminal recurrence regression coverage.
This was referenced Aug 31, 2026
Merged
fix(alertmanager): record close authorship instead of inferring it from resolvedAt (BLO-31736)
#1648
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-24177
severity=nonedelivery evidence asdoneand unassigned.Verification
git diff --checkclean.tsc: not found).Acceptance criteria