From a89c08a59e408dc42c2b95aa870a56e2087041e9 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:17:57 +0100 Subject: [PATCH] Drop recovery colours and check marks from moved alert closes A second review found the moved-alert text was honest but the visuals still said recovered: Discord kept its green embed, both Teams cards kept recovery green, Mattermost led with a check emoji, and a grouped ntfy batch holding a moved alert kept the green check tag. A check mark on a phone is read before the sentence under it. Resolved webhook data now carries NotRecovered when any close in the delivery was not a recovery. The built-in templates switch to neutral colours and an arrow when it is set, and ntfy uses the arrow tag for a single alert or a batch. Titles still say Resolved, which stays true for the closed alert, and ordinary recoveries keep their green. The render tests now use an isolated data directory, stop their managers, and hand the captured request over a channel, so they no longer touch a shared queue or race on the captured body. --- .../v6/internal/subsystems/notifications.md | 26 ++-- internal/notifications/notifications.go | 19 ++- .../notifications/resolved_handover_test.go | 144 ++++++++++++------ internal/notifications/webhook_enhanced.go | 4 + internal/notifications/webhook_templates.go | 8 +- 5 files changed, 141 insertions(+), 60 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index bb8bba2518..0afc28d582 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -1229,16 +1229,22 @@ receives `resolve` for the alert's dedup key. Only the wording changes. agent for the current reading.") in place of " on is now healthy" for every templated webhook `.Message`. Grouped lists append the short outcome ("moved to pve1 (Host Agent)") to the moved entry. ntfy titles a -single moved alert `MOVED: ` and swaps the green check tag for -`arrow_right`. Email and Apprise title it `Pulse alert moved: ` and -add the summary after the breach message. The resolution rides the Alert JSON, -so it survives the delivery queue round trip and appears in the default JSON -payload's `alerts` array. Built-in template titles still say Resolved, which is -accurate for the closed alert, and a custom template that hard-codes recovery -text keeps it. Issue #1682 is the precedent: a recipient called "is now -healthy" a false factual claim when an alert closed without recovering. +single moved alert `MOVED: `. Email and Apprise title it `Pulse alert +moved: ` and add the summary after the breach message. Recovery +visuals follow the same rule: when any close in a resolved delivery was not a +recovery, `WebhookPayloadData.NotRecovered` is set and the built-in templates +drop them (Discord's green embed becomes the neutral blue, both Teams cards +lose recovery green, Mattermost's check emoji becomes `:arrow_right:`), and +ntfy swaps its green check tag for `arrow_right`, for a single alert or a batch. +Built-in titles still say Resolved, which is accurate for the closed alert; +custom templates can read `.NotRecovered`, and one that hard-codes recovery +text keeps it. The resolution rides the Alert JSON, so it survives the delivery +queue round trip and appears in the default JSON payload's `alerts` array. +Issue #1682 is the precedent: a recipient called "is now healthy" a false +factual claim when an alert closed without recovering. Verification: `resolved_handover_test.go` drives the firing, its receipt and the queued close through the normal entry points for ntfy, and renders Discord, -PagerDuty, a grouped Slack list and email/Apprise content, plus the unchanged -ordinary recovery wording. +both Teams cards, Mattermost, PagerDuty, a grouped Slack list, a grouped ntfy +batch and email/Apprise content, plus the unchanged ordinary recovery wording +and visuals. diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 93ae16e0f3..71f467b967 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -453,6 +453,17 @@ func resolvedAlertNotRecoveredSummary(alert *alerts.Alert) string { return alert.Resolution.Describe(label) } +// resolvedAlertsIncludeNotRecovered reports whether any close in a resolved +// notification was not a recovery, so recovery visuals must not be used. +func resolvedAlertsIncludeNotRecovered(alertList []*alerts.Alert) bool { + for _, alert := range alertList { + if alert != nil && alert.Resolution.Outcome() != "" { + return true + } + } + return false +} + // resolvedAlertListEntry names one alert in a grouped recovery list. func resolvedAlertListEntry(alert *alerts.Alert) string { entry := fmt.Sprintf("%s on %s", alert.ResourceName, alert.Node) @@ -3018,6 +3029,7 @@ func (n *NotificationManager) sendResolvedWebhook(webhook WebhookConfig, alertLi data.ResolvedAtISO = resolvedAt.Format(time.RFC3339) data.Duration = formatWebhookDuration(resolvedAt.Sub(alert.StartTime)) data.Message = resolvedAlertMessage(alert) + data.NotRecovered = resolvedAlertsIncludeNotRecovered(alertList) data.AlertCount = len(alertList) data.Alerts = alertList if len(alertList) > 1 { @@ -3074,16 +3086,19 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale } // Build plain-text body and title. A single alert that moved rather than - // recovered says so in both, and drops the green check. + // recovered says so in both, and no batch holding such a close carries + // the green check. var body strings.Builder title := "RESOLVED" tags := "white_check_mark,pulse,resolved" + if resolvedAlertsIncludeNotRecovered(alertList) { + tags = "arrow_right,pulse,resolved" + } if len(alertList) == 1 && alertList[0] != nil { a := alertList[0] title = fmt.Sprintf("RESOLVED: %s", a.ResourceName) if summary := resolvedAlertNotRecoveredSummary(a); summary != "" { title = fmt.Sprintf("MOVED: %s", a.ResourceName) - tags = "arrow_right,pulse,resolved" body.WriteString(summary) } else { fmt.Fprintf(&body, "Resolved: %s", resolvedAlertMessage(a)) diff --git a/internal/notifications/resolved_handover_test.go b/internal/notifications/resolved_handover_test.go index b980c6fb56..f144a178be 100644 --- a/internal/notifications/resolved_handover_test.go +++ b/internal/notifications/resolved_handover_test.go @@ -99,64 +99,108 @@ func TestMovedAlertCloseIsDeliveredAsMovedNotRecovered(t *testing.T) { } } -func TestMovedAlertCloseRendersHonestlyOnEveryChannel(t *testing.T) { - capture := func(t *testing.T, service string) map[string]any { - t.Helper() - var gotBody []byte - server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotBody, _ = io.ReadAll(r.Body) - w.WriteHeader(http.StatusOK) - })) - defer server.Close() - nm := NewNotificationManager("https://pulse.local") - _ = nm.UpdateAllowedPrivateCIDRs("127.0.0.1") - webhook := WebhookConfig{Name: service, URL: server.URL + "/hook", Enabled: true, Service: service, - CustomFields: map[string]string{"routing_key": "rk"}} - if err := nm.sendResolvedWebhook(webhook, []*alerts.Alert{movedNodeMemoryAlert()}, time.Now()); err != nil { - t.Fatalf("sendResolvedWebhook %s: %v", service, err) - } - var payload map[string]any - if err := json.Unmarshal(gotBody, &payload); err != nil { - t.Fatalf("unmarshal %s payload: %v", service, err) - } - return payload +// captureResolvedWebhook renders one resolved delivery through an isolated +// manager and returns what the destination received. +func captureResolvedWebhook(t *testing.T, service string, alertList []*alerts.Alert) ([]byte, http.Header) { + t.Helper() + type request struct { + body []byte + header http.Header } + received := make(chan request, 1) + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + received <- request{body: body, header: r.Header.Clone()} + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + nm := NewNotificationManagerWithDataDir("https://pulse.local", t.TempDir()) + t.Cleanup(nm.Stop) + if err := nm.UpdateAllowedPrivateCIDRs("127.0.0.1"); err != nil { + t.Fatal(err) + } + webhook := WebhookConfig{Name: service, URL: server.URL + "/hook", Enabled: true, Service: service, + CustomFields: map[string]string{"routing_key": "rk"}} + if err := nm.sendResolvedWebhook(webhook, alertList, time.Now()); err != nil { + t.Fatalf("sendResolvedWebhook %s: %v", service, err) + } + select { + case got := <-received: + return got.body, got.header + case <-time.After(5 * time.Second): + t.Fatalf("%s destination received nothing", service) + return nil, nil + } +} - t.Run("discord says moved", func(t *testing.T) { - embed := capture(t, "discord")["embeds"].([]any)[0].(map[string]any) +func decodeResolvedPayload(t *testing.T, service string, body []byte) map[string]any { + t.Helper() + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("unmarshal %s payload: %v\n%s", service, err, body) + } + return payload +} + +func TestMovedAlertCloseRendersHonestlyOnEveryChannel(t *testing.T) { + recovered := &alerts.Alert{ID: "vm-cpu", Type: "cpu", ResourceName: "web-vm-01", Node: "pve1", StartTime: time.Now().Add(-time.Minute)} + + t.Run("discord says moved without the recovery colour", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "discord", []*alerts.Alert{movedNodeMemoryAlert()}) + embed := decodeResolvedPayload(t, "discord", body)["embeds"].([]any)[0].(map[string]any) description, _ := embed["description"].(string) if !strings.Contains(description, movedSummary) || strings.Contains(description, "healthy") { t.Fatalf("description = %q, want the handover summary", description) } + if embed["color"] != float64(3447003) { + t.Fatalf("embed color = %v, want the neutral 3447003, not recovery green", embed["color"]) + } + }) + + t.Run("teams cards drop recovery green", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "teams", []*alerts.Alert{movedNodeMemoryAlert()}) + if got := decodeResolvedPayload(t, "teams", body)["themeColor"]; got != "0076D7" { + t.Fatalf("teams themeColor = %v, want 0076D7", got) + } + body, _ = captureResolvedWebhook(t, "teams-adaptive", []*alerts.Alert{movedNodeMemoryAlert()}) + if !strings.Contains(string(body), `"color": "Accent"`) || strings.Contains(string(body), `"color": "Good"`) { + t.Fatalf("teams adaptive card = %s, want the Accent title colour", body) + } + }) + + t.Run("mattermost drops the check mark", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "mattermost", []*alerts.Alert{movedNodeMemoryAlert()}) + text, _ := decodeResolvedPayload(t, "mattermost", body)["text"].(string) + if strings.Contains(text, ":white_check_mark:") || !strings.Contains(text, ":arrow_right: **RESOLVED**") || !strings.Contains(text, movedSummary) { + t.Fatalf("mattermost text = %q", text) + } }) t.Run("pagerduty still closes the incident", func(t *testing.T) { - payload := capture(t, "pagerduty") + body, _ := captureResolvedWebhook(t, "pagerduty", []*alerts.Alert{movedNodeMemoryAlert()}) + payload := decodeResolvedPayload(t, "pagerduty", body) if payload["event_action"] != "resolve" || payload["dedup_key"] != "pve1-memory" { t.Fatalf("payload = %v, want a resolve for the node alert's incident", payload) } }) t.Run("grouped list names the move", func(t *testing.T) { - var gotBody []byte - server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotBody, _ = io.ReadAll(r.Body) - w.WriteHeader(http.StatusOK) - })) - defer server.Close() - nm := NewNotificationManager("https://pulse.local") - _ = nm.UpdateAllowedPrivateCIDRs("127.0.0.1") - recovered := &alerts.Alert{ID: "vm-cpu", Type: "cpu", ResourceName: "web-vm-01", Node: "pve1", StartTime: time.Now().Add(-time.Minute)} - webhook := WebhookConfig{Name: "slack", URL: server.URL + "/hook", Enabled: true, Service: "slack"} - if err := nm.sendResolvedWebhook(webhook, []*alerts.Alert{movedNodeMemoryAlert(), recovered}, time.Now()); err != nil { - t.Fatalf("sendResolvedWebhook: %v", err) - } - body := string(gotBody) - if !strings.Contains(body, "pve1 on pve1, moved to pve1 (Host Agent)") || !strings.Contains(body, "web-vm-01 on pve1") { + body, _ := captureResolvedWebhook(t, "slack", []*alerts.Alert{movedNodeMemoryAlert(), recovered}) + if !strings.Contains(string(body), "pve1 on pve1, moved to pve1 (Host Agent)") || !strings.Contains(string(body), "web-vm-01 on pve1") { t.Fatalf("grouped body = %s, want the moved alert marked and the recovery listed", body) } }) + t.Run("grouped ntfy batch drops the check mark", func(t *testing.T) { + body, header := captureResolvedWebhook(t, "ntfy", []*alerts.Alert{movedNodeMemoryAlert(), recovered}) + if tags := header.Get("Tags"); strings.Contains(tags, "white_check_mark") || !strings.Contains(tags, "arrow_right") { + t.Fatalf("ntfy tags = %q, want no check mark on a batch holding a move", tags) + } + if !strings.Contains(string(body), "- pve1 on pve1, moved to pve1 (Host Agent)\n") { + t.Fatalf("ntfy body = %q", body) + } + }) + t.Run("email and apprise say moved", func(t *testing.T) { title, htmlBody, textBody := buildResolvedNotificationContent([]*alerts.Alert{movedNodeMemoryAlert()}, time.Now(), "") if title != "Pulse alert moved: pve1" { @@ -170,14 +214,26 @@ func TestMovedAlertCloseRendersHonestlyOnEveryChannel(t *testing.T) { } }) - t.Run("ordinary recovery is unchanged", func(t *testing.T) { - recovered := movedNodeMemoryAlert() - recovered.Resolution = nil - if got := resolvedAlertMessage(recovered); got != "pve1 on pve1 is now healthy" { + t.Run("ordinary recovery keeps its wording and visuals", func(t *testing.T) { + plain := movedNodeMemoryAlert() + plain.Resolution = nil + if got := resolvedAlertMessage(plain); got != "pve1 on pve1 is now healthy" { t.Fatalf("resolvedAlertMessage() = %q", got) } - if title, _, _ := buildResolvedNotificationContent([]*alerts.Alert{recovered}, time.Now(), ""); title != "Pulse alert resolved: pve1" { + if title, _, _ := buildResolvedNotificationContent([]*alerts.Alert{plain}, time.Now(), ""); title != "Pulse alert resolved: pve1" { t.Fatalf("title = %q", title) } + body, _ := captureResolvedWebhook(t, "discord", []*alerts.Alert{plain}) + if embed := decodeResolvedPayload(t, "discord", body)["embeds"].([]any)[0].(map[string]any); embed["color"] != float64(3066993) { + t.Fatalf("ordinary recovery embed color = %v, want green 3066993", embed["color"]) + } + body, _ = captureResolvedWebhook(t, "mattermost", []*alerts.Alert{plain}) + if text, _ := decodeResolvedPayload(t, "mattermost", body)["text"].(string); !strings.Contains(text, ":white_check_mark: **RESOLVED**") { + t.Fatalf("ordinary mattermost text = %q", text) + } + _, header := captureResolvedWebhook(t, "ntfy", []*alerts.Alert{plain}) + if tags := header.Get("Tags"); tags != "white_check_mark,pulse,resolved" { + t.Fatalf("ordinary ntfy tags = %q", tags) + } }) } diff --git a/internal/notifications/webhook_enhanced.go b/internal/notifications/webhook_enhanced.go index 5c390fd1c6..c586c2e9fb 100644 --- a/internal/notifications/webhook_enhanced.go +++ b/internal/notifications/webhook_enhanced.go @@ -65,6 +65,10 @@ type WebhookPayloadData struct { Event string // "alert" or "resolved" ResolvedAt string // RFC3339 timestamp when alert was resolved ResolvedAtISO string // Same as ResolvedAt (alias for template consistency) + // NotRecovered marks a resolved notification in which at least one close + // was not a recovery (an alert that moved to a linked Pulse agent), so + // templates can drop recovery colours and check marks. + NotRecovered bool // Tenant identity (multi-tenant orgs and MSP client runtimes). // Empty on plain single-tenant installs. diff --git a/internal/notifications/webhook_templates.go b/internal/notifications/webhook_templates.go index 8c9f8db4f0..5e4b2bf152 100644 --- a/internal/notifications/webhook_templates.go +++ b/internal/notifications/webhook_templates.go @@ -56,7 +56,7 @@ func GetWebhookTemplates() []WebhookTemplate { "embeds": [{ "title": "Resolved: {{.ResourceName | jsonString}}", "description": "{{.Message | jsonString}}", - "color": 3066993, + "color": {{if .NotRecovered}}3447003{{else}}3066993{{end}}, "fields": [ {"name": "Resource", "value": "{{.ResourceName | jsonString}}", "inline": true}, {"name": "Node", "value": "{{.Node | jsonString}}", "inline": true}, @@ -241,7 +241,7 @@ func GetWebhookTemplates() []WebhookTemplate { ResolvedPayloadTemplate: `{ "@type": "MessageCard", "@context": "http://schema.org/extensions", - "themeColor": "2DC72D", + "themeColor": "{{if .NotRecovered}}0076D7{{else}}2DC72D{{end}}", "summary": "Resolved: {{.ResourceName | jsonString}}", {{if .Mention}}"text": "{{.Mention | jsonString}}",{{end}} "sections": [{ @@ -372,7 +372,7 @@ func GetWebhookTemplates() []WebhookTemplate { "text": "Resolved: {{.ResourceName | jsonString}}", "weight": "Bolder", "size": "Large", - "color": "Good" + "color": "{{if .NotRecovered}}Accent{{else}}Good{{end}}" }, { "type": "TextBlock", @@ -521,7 +521,7 @@ View in Pulse: {{.Instance}}`, ResolvedPayloadTemplate: `{ "username": "Pulse Monitoring", "icon_url": "https://raw.githubusercontent.com/rcourtman/Pulse/main/frontend-modern/public/android-chrome-192x192.png", - "text": "{{if .Mention}}{{.Mention | jsonString}}\n\n{{end}}:white_check_mark: **RESOLVED**\n\n**{{.ResourceName | jsonString}}** on **{{.Node | jsonString}}**\n\n{{.Message | jsonString}}\n\n| Detail | Value |\n|:-------|:------|\n| Resource | {{.ResourceName | jsonString}} |\n| Node | {{.Node | jsonString}} |\n| Type | {{.Type | title | jsonString}} |\n| Duration | {{.Duration | jsonString}} |\n| Resolved At | {{.ResolvedAt | jsonString}} |\n| Alert Identifier | {{.ID | jsonString}} |" + "text": "{{if .Mention}}{{.Mention | jsonString}}\n\n{{end}}{{if .NotRecovered}}:arrow_right:{{else}}:white_check_mark:{{end}} **RESOLVED**\n\n**{{.ResourceName | jsonString}}** on **{{.Node | jsonString}}**\n\n{{.Message | jsonString}}\n\n| Detail | Value |\n|:-------|:------|\n| Resource | {{.ResourceName | jsonString}} |\n| Node | {{.Node | jsonString}} |\n| Type | {{.Type | title | jsonString}} |\n| Duration | {{.Duration | jsonString}} |\n| Resolved At | {{.ResolvedAt | jsonString}} |\n| Alert Identifier | {{.ID | jsonString}} |" }`, Instructions: "1. In Mattermost, go to Integrations > Incoming Webhooks\n2. Create a new webhook and select the channel\n3. Copy the webhook URL and paste it here\n\nNote: This template uses Markdown formatting which is fully supported by Mattermost.", },