diff --git a/pkg/common/report_senders_test.go b/pkg/common/report_senders_test.go index be060fd..0244c35 100644 --- a/pkg/common/report_senders_test.go +++ b/pkg/common/report_senders_test.go @@ -88,9 +88,14 @@ func pngAttachment() core.Attachment { func TestSlackProvider_SendAttachment(t *testing.T) { var uploadedBody []byte var completeForm string + var joined bool var srv *httptest.Server srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { + case strings.HasSuffix(r.URL.Path, "/conversations.join"): + joined = true + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"ok":true,"channel":{"id":"C123"}}`) case strings.HasSuffix(r.URL.Path, "/files.getUploadURLExternal"): w.Header().Set("Content-Type", "application/json") // upload_url points back at this server's /upload route. @@ -115,6 +120,9 @@ func TestSlackProvider_SendAttachment(t *testing.T) { if err := p.SendAttachment(&m.Incident{}, pngAttachment()); err != nil { t.Fatalf("SendAttachment: %v", err) } + if !joined { + t.Fatal("expected a best-effort conversations.join before upload") + } if !strings.Contains(string(uploadedBody), "FAKEPNGDATA") { t.Fatalf("upload did not carry the PNG bytes; got %q", uploadedBody) } @@ -123,6 +131,56 @@ func TestSlackProvider_SendAttachment(t *testing.T) { } } +// TestSlackProvider_SendAttachment_NotInChannel verifies that when the upload +// is rejected with not_in_channel (the bot isn't a member), the failure is +// wrapped in an operator-actionable message — and that no token/secret leaks +// into the error. +func TestSlackProvider_SendAttachment_NotInChannel(t *testing.T) { + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/conversations.join"): + // Simulate a private channel / missing scope: join fails, and we + // still proceed to the upload (which then gets rejected). + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"ok":false,"error":"method_not_supported_for_channel_type"}`) + case strings.HasSuffix(r.URL.Path, "/files.getUploadURLExternal"): + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"ok":true,"upload_url":"`+srv.URL+`/upload","file_id":"F1"}`) + case strings.HasSuffix(r.URL.Path, "/upload"): + w.WriteHeader(http.StatusOK) + case strings.HasSuffix(r.URL.Path, "/files.completeUploadExternal"): + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"ok":false,"error":"not_in_channel"}`) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + client := slack.New("xoxb-secret-token", slack.OptionAPIURL(srv.URL+"/")) + p := &SlackProvider{client: client, channelID: "C123"} + + err := p.SendAttachment(&m.Incident{}, pngAttachment()) + if err == nil { + t.Fatal("expected an error when the upload is rejected with not_in_channel") + } + msg := err.Error() + for _, want := range []string{ + "not a member of channel C123", + "channels:join", + "/invite", + } { + if !strings.Contains(msg, want) { + t.Fatalf("actionable error missing %q; got: %s", want, msg) + } + } + // The token/secret must never appear in the surfaced error. + if strings.Contains(msg, "xoxb-secret-token") { + t.Fatalf("error leaked the bot token: %s", msg) + } +} + // --- Telegram (sendPhoto multipart) ---------------------------------------- func TestTelegramProvider_SendAttachment(t *testing.T) { diff --git a/pkg/common/slack.go b/pkg/common/slack.go index 3d95f3a..669b7a6 100644 --- a/pkg/common/slack.go +++ b/pkg/common/slack.go @@ -3,6 +3,7 @@ package common import ( "bytes" "context" + "errors" "fmt" "path/filepath" "strings" @@ -40,11 +41,26 @@ func (s *SlackProvider) Name() string { return "slack" } // the slack-go three-step external-upload flow (files.getUploadURLExternal // → upload → files.completeUploadExternal), which is the modern replacement // for the retired files.upload endpoint. +// +// Unlike alerts (which post a message via chat.postMessage and only require +// the bot to be able to post to a public channel), sharing a file into a +// channel requires the bot to be a *member*. So before uploading we make a +// best-effort attempt to join the channel — this self-heals the common +// public-channel case when the bot has the channels:join scope, and is +// harmless when it can't (private channel / missing scope), in which case we +// still try the upload and surface an actionable error if it's rejected. func (s *SlackProvider) SendAttachment(i *m.Incident, att core.Attachment) error { if len(att.Data) == 0 { return fmt.Errorf("slack: empty attachment") } - _, err := s.client.UploadFileContext(context.Background(), slack.UploadFileParameters{ + ctx := context.Background() + + // Best-effort auto-join: ignore the error. Joining only works for public + // channels when the bot holds the channels:join scope; it legitimately + // fails otherwise, and we still attempt the upload below. + _, _, _, _ = s.client.JoinConversationContext(ctx, s.channelID) + + _, err := s.client.UploadFileContext(ctx, slack.UploadFileParameters{ Reader: bytes.NewReader(att.Data), FileSize: len(att.Data), Filename: att.Filename, @@ -53,11 +69,29 @@ func (s *SlackProvider) SendAttachment(i *m.Incident, att core.Attachment) error Channel: s.channelID, }) if err != nil { + if isNotInChannel(err) { + return fmt.Errorf("slack upload failed: the bot is not a member of channel %s — invite it in Slack (\"/invite @\") or grant the channels:join scope so it can upload report files (alerts post messages and don't need this)", s.channelID) + } return fmt.Errorf("slack upload: %w", err) } return nil } +// isNotInChannel reports whether a Slack API error is the not_in_channel +// rejection returned when the bot tries to share a file into a channel it +// hasn't joined. It inspects the typed slack.SlackErrorResponse first and +// falls back to a substring match on the error string. +func isNotInChannel(err error) bool { + if err == nil { + return false + } + var slackErr slack.SlackErrorResponse + if errors.As(err, &slackErr) && slackErr.Err == "not_in_channel" { + return true + } + return strings.Contains(err.Error(), "not_in_channel") +} + // SendAlert determines whether to process a resolved or unresolved incident func (s *SlackProvider) SendAlert(i *m.Incident) error { if i.Resolved { diff --git a/pkg/controllers/incidents_admin.go b/pkg/controllers/incidents_admin.go index f2e44e3..8945120 100644 --- a/pkg/controllers/incidents_admin.go +++ b/pkg/controllers/incidents_admin.go @@ -33,6 +33,7 @@ func NewIncidentAdminController() *IncidentAdminController { // // GET /api/admin/incidents list (newest first; ?limit=NN) // GET /api/admin/incidents/search full-text search (?q=&limit=NN) +// GET /api/admin/incidents/counts cheap per-origin × per-status tally // GET /api/admin/incidents/intake-settings read intake settings // PUT /api/admin/incidents/intake-settings update intake settings // GET /api/admin/incidents/:id single record @@ -47,6 +48,10 @@ func (i *IncidentAdminController) Register(router fiber.Router) { // /search MUST be registered before /:id so the literal path is not // swallowed by the :id parameter route. g.Get("/search", i.search) + // /counts is the cheap, rows-free per-origin × per-status tally the Now + // page and header badge read; like /search it MUST precede /:id so the + // literal path is not captured as an incident id. + g.Get("/counts", i.counts) // /intake-settings likewise MUST precede /:id so the literal settings // path is not captured as an incident id. g.Get("/intake-settings", i.getIntakeSettings) @@ -90,7 +95,7 @@ func (i *IncidentAdminController) list(c *fiber.Ctx) error { // never load the whole table. Postgres (unbounded history) and the // file/memory backends (already capped) all implement it. if pager, ok := store.(storage.IncidentPager); ok { - counts, err := pager.CountIncidents() + counts, err := pager.CountIncidentsByStatus() if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": err.Error()}) } @@ -188,7 +193,7 @@ func (i *IncidentAdminController) search(c *fiber.Ctx) error { // query plus one count query — never the whole match set. Postgres // implements it; it is the only unbounded Searcher. if sp, ok := store.(storage.IncidentSearchPager); ok { - counts, err := sp.CountIncidentsMatching(query) + counts, err := sp.CountIncidentsMatchingByStatus(query) if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": err.Error()}) } @@ -210,6 +215,31 @@ func (i *IncidentAdminController) search(c *fiber.Ctx) error { return c.JSON(incidentListResponse(recs, origin, c.Query("page"), c.Query("page_size"), parseLimit(c.Query("limit")))) } +// counts returns the whole-set per-origin × per-status incident tally in one +// cheap, rows-free response so the Now page and the header badge can show +// authoritative numbers WITHOUT loading a page of rows. Preferred path is the +// bounded pager's single COUNT query; the fallback (a backend with no pager) +// tallies a materialized window. The shape matches the list response's counts +// object (top-level unresolved + by_status), so both surfaces read one type. +func (i *IncidentAdminController) counts(c *fiber.Ctx) error { + store := services.Storage() + if store == nil { + return c.JSON(countsMap(storage.IncidentStatusCounts{})) + } + if pager, ok := store.(storage.IncidentPager); ok { + sc, err := pager.CountIncidentsByStatus() + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": err.Error()}) + } + return c.JSON(countsMap(sc)) + } + recs, err := store.ListIncidents(0) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": err.Error()}) + } + return c.JSON(countsMap(storage.StatusCountsOf(recs))) +} + func (i *IncidentAdminController) get(c *fiber.Ctx) error { store := services.Storage() if store == nil { @@ -387,6 +417,49 @@ func originCountsMap(c storage.IncidentCounts) fiber.Map { } } +// perOriginMap renders one status bucket across origins into the {ai_detect, +// webhook, total} shape the UI reads for a single status row. +func perOriginMap(ai, webhook, total int) fiber.Map { + return fiber.Map{"ai_detect": ai, "webhook": webhook, "total": total} +} + +// statusCountsMap renders the whole-set per-origin × per-status tally into the +// nested by_status shape the UI consumes: one {ai_detect, webhook, total} +// object per status bucket (open / acked / resolved / all). It is additive to +// the existing {ai_detect, webhook, total} counts object — consumers that read +// only the top-level unresolved counts are unaffected. +func statusCountsMap(c storage.IncidentStatusCounts) fiber.Map { + return fiber.Map{ + "open": perOriginMap(c.AIDetect.Open, c.Webhook.Open, c.Total.Open), + "acked": perOriginMap(c.AIDetect.Acked, c.Webhook.Acked, c.Total.Acked), + "resolved": perOriginMap(c.AIDetect.Resolved, c.Webhook.Resolved, c.Total.Resolved), + "all": perOriginMap(c.AIDetect.Total, c.Webhook.Total, c.Total.Total), + } +} + +// unresolvedCounts derives the open-work (unresolved = open + acked) per-origin +// tally from the full per-status breakdown, so the back-compat top-level counts +// object stays identical to what CountIncidents returned while the whole +// response is built from ONE by-status count. +func unresolvedCounts(c storage.IncidentStatusCounts) storage.IncidentCounts { + return storage.IncidentCounts{ + AIDetect: c.AIDetect.Open + c.AIDetect.Acked, + Webhook: c.Webhook.Open + c.Webhook.Acked, + Total: c.Total.Open + c.Total.Acked, + } +} + +// countsMap renders the full count object the list / search / counts responses +// carry: the back-compat top-level unresolved (open-work) per-origin tally PLUS +// the additive by_status breakdown, both from ONE per-status count. Existing +// consumers read ai_detect/webhook/total unchanged; the count surfaces read +// by_status for the authoritative per-origin × per-status numbers. +func countsMap(c storage.IncidentStatusCounts) fiber.Map { + m := originCountsMap(unresolvedCounts(c)) + m["by_status"] = statusCountsMap(c) + return m +} + // filteredTotal returns the unresolved count that matches the active origin // filter, derived from the whole-set breakdown so the badge for the active // tab shows that tab's open count, while the counts object stays the full @@ -417,14 +490,14 @@ func filteredTotal(c storage.IncidentCounts, origin string) int { // driven off PAGE-FULLNESS: a full page (len == size) implies at least one // more page, an underfull page is the last one. This lets the operator page // past the unresolved count — and past row 1000 — through the entire history. -func pagedIncidentResponse(recs []*storage.IncidentRecord, counts storage.IncidentCounts, origin string, offset, size, page int) fiber.Map { - total := filteredTotal(counts, origin) +func pagedIncidentResponse(recs []*storage.IncidentRecord, counts storage.IncidentStatusCounts, origin string, offset, size, page int) fiber.Map { + total := filteredTotal(unresolvedCounts(counts), origin) out := make([]fiber.Map, 0, len(recs)) for _, r := range recs { out = append(out, summarize(r)) } resp := fiber.Map{ - "counts": originCountsMap(counts), + "counts": countsMap(counts), "total": total, "incidents": out, "offset": offset, @@ -476,6 +549,10 @@ func pagedAnalysisResponse(recs []*storage.AnalysisRecord, total, offset, size, // back-compat shape existing callers depend on. func incidentListResponse(recs []*storage.IncidentRecord, origin, pageParam, pageSizeParam string, limit int) fiber.Map { counts := originCounts(recs) + // by_status is the authoritative per-origin × per-status breakdown; on this + // fallback path (a backend with no bounded pager) it is tallied over the + // materialized window so the count surfaces still read the same shape. + counts["by_status"] = statusCountsMap(storage.StatusCountsOf(recs)) recs = filterByOrigin(recs, origin) total := len(recs) diff --git a/pkg/controllers/incidents_admin_test.go b/pkg/controllers/incidents_admin_test.go index fbe1899..63092da 100644 --- a/pkg/controllers/incidents_admin_test.go +++ b/pkg/controllers/incidents_admin_test.go @@ -5,6 +5,7 @@ import ( "io" "net/http/httptest" "testing" + "time" "github.com/VersusControl/versus-incident/pkg/services" "github.com/VersusControl/versus-incident/pkg/storage" @@ -28,6 +29,7 @@ func TestResolveRouteRegistered(t *testing.T) { }{ {"GET", "/api/admin/incidents/"}, {"GET", "/api/admin/incidents/search"}, + {"GET", "/api/admin/incidents/counts"}, {"GET", "/api/admin/incidents/intake-settings"}, {"PUT", "/api/admin/incidents/intake-settings"}, {"GET", "/api/admin/incidents/:id"}, @@ -170,6 +172,22 @@ func TestSearchSupportedReturnsResults(t *testing.T) { } } +// perOriginJSON mirrors one {ai_detect, webhook, total} status bucket. +type perOriginJSON struct { + AIDetect int `json:"ai_detect"` + Webhook int `json:"webhook"` + Total int `json:"total"` +} + +// byStatusJSON mirrors the additive counts.by_status breakdown: one per-origin +// bucket per status. +type byStatusJSON struct { + Open perOriginJSON `json:"open"` + Acked perOriginJSON `json:"acked"` + Resolved perOriginJSON `json:"resolved"` + All perOriginJSON `json:"all"` +} + // incidentListResp mirrors the JSON shape returned by the list/search // endpoints, including the additive origin counts and pagination meta. type incidentListResp struct { @@ -178,9 +196,10 @@ type incidentListResp struct { Origin string `json:"origin"` } `json:"incidents"` Counts struct { - AIDetect int `json:"ai_detect"` - Webhook int `json:"webhook"` - Total int `json:"total"` + AIDetect int `json:"ai_detect"` + Webhook int `json:"webhook"` + Total int `json:"total"` + ByStatus byStatusJSON `json:"by_status"` } `json:"counts"` Total int `json:"total"` Page int `json:"page"` @@ -337,3 +356,108 @@ func TestListPagination(t *testing.T) { t.Fatalf("ai_detect page 2 = rows:%d total:%d, want 2/4", len(ai2.Incidents), ai2.Total) } } + +// seedStatusStore returns a memory store with a known per-origin × per-status +// spread so the by_status breakdown has a non-trivial truth to match: +// +// ai_detect: 2 open, 1 acked, 1 resolved (total 4) +// webhook: 1 open, 1 acked, 2 resolved (total 4, incl. a legacy row) +func seedStatusStore(t *testing.T) storage.Provider { + t.Helper() + mem := storage.NewMemory() + acked := time.Unix(1_700_000_000, 0).UTC() + recs := []*storage.IncidentRecord{ + {ID: "ai-open-1", Origin: storage.OriginAIDetect, Source: "agent"}, + {ID: "ai-open-2", Origin: storage.OriginAIDetect, Source: "agent"}, + {ID: "ai-acked", Origin: storage.OriginAIDetect, Source: "agent", AckedAt: &acked}, + {ID: "ai-resolved", Origin: storage.OriginAIDetect, Source: "agent", Resolved: true}, + {ID: "wh-open", Origin: storage.OriginWebhook, Source: "webhook"}, + {ID: "wh-acked", Origin: storage.OriginWebhook, Source: "sns", AckedAt: &acked}, + {ID: "wh-resolved", Origin: storage.OriginWebhook, Source: "webhook", Resolved: true}, + {ID: "legacy-resolved", Source: "sqs", Resolved: true}, // derives webhook + } + for _, r := range recs { + if err := mem.SaveIncident(r); err != nil { + t.Fatalf("SaveIncident: %v", err) + } + } + return mem +} + +func wantByStatus() byStatusJSON { + return byStatusJSON{ + Open: perOriginJSON{AIDetect: 2, Webhook: 1, Total: 3}, + Acked: perOriginJSON{AIDetect: 1, Webhook: 1, Total: 2}, + Resolved: perOriginJSON{AIDetect: 1, Webhook: 2, Total: 3}, + All: perOriginJSON{AIDetect: 4, Webhook: 4, Total: 8}, + } +} + +// TestCountsEndpointByStatus verifies the dedicated /counts endpoint returns +// the authoritative per-origin × per-status breakdown — the single source of +// truth the header badge and the Now page read instead of tallying a bounded +// page of rows. +func TestCountsEndpointByStatus(t *testing.T) { + t.Cleanup(func() { services.SetStorage(nil) }) + services.SetStorage(seedStatusStore(t)) + ctrl := NewIncidentAdminController() + + app := fiber.New() + app.Get("/counts", ctrl.counts) + resp, err := app.Test(httptest.NewRequest("GET", "/counts", nil)) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != fiber.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } + body, _ := io.ReadAll(resp.Body) + var got struct { + AIDetect int `json:"ai_detect"` + Webhook int `json:"webhook"` + Total int `json:"total"` + ByStatus byStatusJSON `json:"by_status"` + } + if err := json.Unmarshal(body, &got); err != nil { + t.Fatalf("unmarshal %q: %v", body, err) + } + + if got.ByStatus != wantByStatus() { + t.Fatalf("by_status = %+v, want %+v", got.ByStatus, wantByStatus()) + } + // The back-compat top-level counts stay unresolved-only (open + acked). + if got.AIDetect != 3 || got.Webhook != 2 || got.Total != 5 { + t.Fatalf("top-level unresolved counts = ai:%d wh:%d total:%d, want 3/2/5", + got.AIDetect, got.Webhook, got.Total) + } +} + +// TestListCarriesByStatus verifies the list response carries the same +// authoritative by_status breakdown, so the Incidents page reads server counts +// off its existing page fetch (no extra request) — and those counts reflect the +// WHOLE set even when only a bounded page of rows is returned. +func TestListCarriesByStatus(t *testing.T) { + t.Cleanup(func() { services.SetStorage(nil) }) + services.SetStorage(seedStatusStore(t)) + ctrl := NewIncidentAdminController() + + // A tiny page so the returned rows are a strict subset of the set; the + // counts must still describe all 8 incidents. + got := doList(t, ctrl, "?page_size=2") + if len(got.Incidents) != 2 { + t.Fatalf("page rows = %d, want 2 (bounded)", len(got.Incidents)) + } + if got.Counts.ByStatus != wantByStatus() { + t.Fatalf("list by_status = %+v, want %+v", got.Counts.ByStatus, wantByStatus()) + } + // Reconciliation invariants: statuses sum to origin total, origins sum to + // the status total — the same guarantee every UI surface now relies on. + bs := got.Counts.ByStatus + if bs.Open.AIDetect+bs.Acked.AIDetect+bs.Resolved.AIDetect != bs.All.AIDetect { + t.Errorf("ai_detect statuses do not sum to its total: %+v", bs) + } + if bs.Open.AIDetect+bs.Open.Webhook != bs.Open.Total { + t.Errorf("open origins do not sum to open total: %+v", bs.Open) + } +} diff --git a/pkg/services/report.go b/pkg/services/report.go index 058ac94..a68b59d 100644 --- a/pkg/services/report.go +++ b/pkg/services/report.go @@ -445,7 +445,14 @@ func RenderIncidentsReport(ctx context.Context, window string) (*core.ReportImag // note where not. Per-channel outcomes are aggregated without // short-circuiting. func SendIncidentsReport(ctx context.Context, opts ReportSendOptions) (*ReportOutcome, error) { - cfg := config.GetConfig() + // Resolve the effective config the SAME way the alert path does so the + // report targets the runtime-overridden channel + token instead of the + // stale YAML config. nil params = no per-incident routing overlay (an + // aggregate report has no single incident to route), but the runtime + // channel override (credentials + channel-id + enable) still applies. + // OSS with no resolver + nil params hits GetConfigForAlert's fast path and + // returns the global cfg unchanged, so community behaviour is identical. + cfg := config.GetConfigForAlert(ctx, nil) providers, err := common.NewAlertProviderFactory(cfg).CreateProviders() if err != nil { return nil, fmt.Errorf("report: build providers: %w", err) diff --git a/pkg/services/report_test.go b/pkg/services/report_test.go index 075de3c..3ea21cb 100644 --- a/pkg/services/report_test.go +++ b/pkg/services/report_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/VersusControl/versus-incident/pkg/agent" + "github.com/VersusControl/versus-incident/pkg/common" "github.com/VersusControl/versus-incident/pkg/config" "github.com/VersusControl/versus-incident/pkg/core" "github.com/VersusControl/versus-incident/pkg/report" @@ -581,3 +582,127 @@ func TestResolveReportChannels_Precedence(t *testing.T) { t.Fatalf("no channel: %v", got) } } + +// reportChannelResolver is a runtime channel-override stub mirroring an +// operator who changed the Slack channel's credentials/channel-id/enable at +// runtime (the hot-reload seam). It overlays only the Slack channel and leaves +// every other channel at its YAML floor. +type reportChannelResolver struct { + enable bool + token string + channelID string +} + +func (r reportChannelResolver) ResolveAlert(_ context.Context, base *config.AlertConfig) bool { + base.Slack.Enable = r.enable + base.Slack.Token = r.token + base.Slack.ChannelID = r.channelID + return true +} + +// TestSendIncidentsReport_HonorsRuntimeChannelOverride proves the report send +// path resolves its channel config the SAME way alerts do (via +// GetConfigForAlert), so a runtime channel override reaches the report's +// providers instead of the stale YAML config — and that with NO resolver the +// report is byte-for-byte identical to the pre-fix behaviour (OSS parity). +func TestSendIncidentsReport_HonorsRuntimeChannelOverride(t *testing.T) { + loadAgentTestConfig(t) + base := config.GetConfigOrNil() + if base == nil { + t.Fatal("global config not loaded") + } + // Known YAML floor: Slack enabled with a YAML channel + token. + savedAlert := base.Alert + t.Cleanup(func() { base.Alert = savedAlert }) + base.Alert = config.AlertConfig{ + Slack: config.SlackConfig{ + Enable: true, + Token: "yaml-token", + ChannelID: "C-YAML", + TemplatePath: "slack.tmpl", + }, + } + + st := windowStore(t) + prevStore := Storage() + SetStorage(st) + t.Cleanup(func() { SetStorage(prevStore) }) + enableReport(t, st, ReportSettings{Enable: true, DefaultChannel: "slack"}) + prevRenderer := ReportRenderer() + SetReportRenderer(fakeRenderer{}) + t.Cleanup(func() { SetReportRenderer(prevRenderer) }) + t.Cleanup(func() { config.SetAlertConfigResolver(nil) }) + + ctx := context.Background() + + // 1. A registered runtime override changes the channel-id + token + enable + // the report's providers are built from — matching the alert path. + t.Run("runtime override reaches report providers", func(t *testing.T) { + config.SetAlertConfigResolver(reportChannelResolver{enable: true, token: "runtime-token", channelID: "C-RUNTIME"}) + t.Cleanup(func() { config.SetAlertConfigResolver(nil) }) + + // The exact resolution SendIncidentsReport performs. + eff := config.GetConfigForAlert(ctx, nil) + if eff.Alert.Slack.ChannelID != "C-RUNTIME" || eff.Alert.Slack.Token != "runtime-token" || !eff.Alert.Slack.Enable { + t.Fatalf("effective slack = %+v, want runtime override (channel-id + token + enable)", eff.Alert.Slack) + } + // The report's providers are built from that resolved config, so the + // Slack provider now targets the overridden channel + token. + providers, err := common.NewAlertProviderFactory(eff).CreateProviders() + if err != nil { + t.Fatalf("build providers: %v", err) + } + if !hasProviderNamed(providers, "slack") { + t.Fatalf("report providers missing overridden slack channel: %v", providerNames(providers)) + } + // The runtime overlay never mutates the global config (golden rule #4). + if base.Alert.Slack.ChannelID != "C-YAML" || base.Alert.Slack.Token != "yaml-token" { + t.Fatalf("global cfg mutated by overlay: %+v", base.Alert.Slack) + } + }) + + // 2. A runtime override that DISABLES the target propagates through the + // real send path (network-free): no provider is built, so the send + // resolves no channel — proving SendIncidentsReport honors the override + // (with the old GetConfig() it would still target the enabled YAML slack). + t.Run("runtime override changes the real send path", func(t *testing.T) { + config.SetAlertConfigResolver(reportChannelResolver{enable: false}) + t.Cleanup(func() { config.SetAlertConfigResolver(nil) }) + + if _, err := SendIncidentsReport(ctx, ReportSendOptions{Window: "today"}); !errors.Is(err, ErrReportNoChannel) { + t.Fatalf("err = %v, want ErrReportNoChannel (runtime override disabled the slack target)", err) + } + }) + + // 3. OSS parity: with NO resolver and nil params, GetConfigForAlert returns + // the GLOBAL cfg pointer unchanged (documented fast path), so a pure-OSS + // report is byte-for-byte identical to the pre-fix GetConfig() behaviour. + t.Run("no resolver uses YAML config unchanged (OSS parity)", func(t *testing.T) { + config.SetAlertConfigResolver(nil) + + if got := config.GetConfigForAlert(ctx, nil); got != config.GetConfig() { + t.Fatal("OSS fast path must return the global cfg pointer unchanged (byte-for-byte parity)") + } + eff := config.GetConfigForAlert(ctx, nil) + if eff.Alert.Slack.ChannelID != "C-YAML" || eff.Alert.Slack.Token != "yaml-token" || !eff.Alert.Slack.Enable { + t.Fatalf("effective slack = %+v, want YAML floor unchanged", eff.Alert.Slack) + } + }) +} + +func hasProviderNamed(providers []core.AlertProvider, name string) bool { + for _, p := range providers { + if p.Name() == name { + return true + } + } + return false +} + +func providerNames(providers []core.AlertProvider) []string { + names := make([]string, 0, len(providers)) + for _, p := range providers { + names = append(names, p.Name()) + } + return names +} diff --git a/pkg/storage/file.go b/pkg/storage/file.go index 061b080..d30f2b3 100644 --- a/pkg/storage/file.go +++ b/pkg/storage/file.go @@ -355,6 +355,17 @@ func (p *fileProvider) CountIncidents() (IncidentCounts, error) { return c, nil } +// CountIncidentsByStatus implements the optional storage.IncidentPager +// capability. The file backend keeps a rolling in-memory cap, so a single +// pass over the slice is cheap; the shared StatusCountsOf helper classifies +// each row via EffectiveOrigin and buckets it by stored status, matching the +// SQL backend exactly. +func (p *fileProvider) CountIncidentsByStatus() (IncidentStatusCounts, error) { + p.mu.RLock() + defer p.mu.RUnlock() + return StatusCountsOf(p.incidents), nil +} + // ListIncidentsPage implements the optional storage.IncidentPager // capability: one bounded, newest-first page over the in-memory slice, // skipping offset matches and returning at most limit rows. The origin diff --git a/pkg/storage/incident_status_counts_test.go b/pkg/storage/incident_status_counts_test.go new file mode 100644 index 0000000..2398df3 --- /dev/null +++ b/pkg/storage/incident_status_counts_test.go @@ -0,0 +1,227 @@ +package storage_test + +// incident_status_counts_test.go — the per-origin × per-status count seam +// (storage.IncidentPager.CountIncidentsByStatus and the search variant). This +// is the storage half of the fix that made the server the single source of +// truth for every displayed count: a cheap COUNT/FILTER breakdown of open / +// acked / resolved per origin, computed WITHOUT loading rows, that must agree +// with the raw stored resolved/acked_at/origin columns even past one page. +// +// Memory and file backends run unconditionally over their capped in-memory +// slice; Postgres is gated on TEST_POSTGRES_DSN. + +import ( + "testing" + "time" + + "github.com/VersusControl/versus-incident/pkg/storage" +) + +type statusSpec struct { + origin string // explicit Origin ("" = legacy, derived from source) + source string + resolved bool + acked bool + want string // expected EffectiveOrigin +} + +// seedStatusRecords saves a deterministic mix of origins and statuses, +// including legacy empty-origin rows (which must classify as webhook). The +// spread of resolved/acked/open across both origins is what lets the test +// prove the breakdown reads the real stored status columns rather than a tally +// of whatever page happened to load. +func seedStatusRecords(t *testing.T, p storage.Provider) []statusSpec { + t.Helper() + base := time.Now().UTC().Add(-2 * time.Hour) + specs := []statusSpec{ + // ai_detect: 3 open, 2 acked, 1 resolved + {storage.OriginAIDetect, "agent", false, false, storage.OriginAIDetect}, + {storage.OriginAIDetect, "agent", false, false, storage.OriginAIDetect}, + {"", "agent:detect", false, false, storage.OriginAIDetect}, // legacy → ai_detect + {storage.OriginAIDetect, "agent", false, true, storage.OriginAIDetect}, + {storage.OriginAIDetect, "agent", false, true, storage.OriginAIDetect}, + {storage.OriginAIDetect, "agent", true, false, storage.OriginAIDetect}, + // webhook: 2 open, 1 acked, 3 resolved (incl. legacy empty-origin) + {storage.OriginWebhook, "webhook", false, false, storage.OriginWebhook}, + {"", "", false, false, storage.OriginWebhook}, // legacy empty → webhook + {storage.OriginWebhook, "sns", false, true, storage.OriginWebhook}, + {storage.OriginWebhook, "webhook", true, false, storage.OriginWebhook}, + {"", "sqs", true, false, storage.OriginWebhook}, // legacy inbound → webhook + {storage.OriginWebhook, "webhook", true, false, storage.OriginWebhook}, + } + for i, s := range specs { + rec := &storage.IncidentRecord{ + ID: string(rune('a' + i)), + Title: "incident", + Origin: s.origin, + Source: s.source, + Resolved: s.resolved, + CreatedAt: base.Add(time.Duration(i) * time.Minute), + } + if s.acked { + ackedAt := rec.CreatedAt.Add(time.Second) + rec.AckedAt = &ackedAt + } + if s.resolved { + resolvedAt := rec.CreatedAt.Add(2 * time.Second) + rec.ResolvedAt = &resolvedAt + } + if got := rec.EffectiveOrigin(); got != s.want { + t.Fatalf("spec %d: EffectiveOrigin = %q, want %q", i, got, s.want) + } + if err := p.SaveIncident(rec); err != nil { + t.Fatalf("SaveIncident %d: %v", i, err) + } + } + return specs +} + +// TestIncidentCountsByStatus proves the per-origin × per-status breakdown +// matches the seeded truth on every backend, and holds the invariants the UI +// relies on: open+acked+resolved == total for each origin, and +// ai_detect+webhook == total for each status. +func TestIncidentCountsByStatus(t *testing.T) { + backends := map[string]func(*testing.T) storage.Provider{ + "memory": newMemoryPager, + "file": newFilePager, + "postgres": func(t *testing.T) storage.Provider { return newTestPostgres(t) }, + } + for name, mk := range backends { + t.Run(name, func(t *testing.T) { + p := mk(t) + seedStatusRecords(t, p) + + pager, ok := p.(storage.IncidentPager) + if !ok { + t.Fatalf("%s backend does not implement storage.IncidentPager", name) + } + got, err := pager.CountIncidentsByStatus() + if err != nil { + t.Fatalf("CountIncidentsByStatus: %v", err) + } + + // Expected from the seed truth table. + want := storage.IncidentStatusCounts{ + AIDetect: storage.StatusCounts{Open: 3, Acked: 2, Resolved: 1, Total: 6}, + Webhook: storage.StatusCounts{Open: 2, Acked: 1, Resolved: 3, Total: 6}, + Total: storage.StatusCounts{Open: 5, Acked: 3, Resolved: 4, Total: 12}, + } + if got != want { + t.Fatalf("CountIncidentsByStatus =\n%+v\nwant\n%+v", got, want) + } + + assertStatusInvariants(t, got) + }) + } +} + +// TestIncidentCountsByStatusMatchesRawFilterPostgres cross-checks the +// COUNT/FILTER breakdown against the raw stored columns on a real Postgres: +// each returned status count must equal a plain SELECT count(*) FILTER over +// vs_incidents. This is the direct check that the "resolved says 0 here but +// 776 there" discrepancy was a client-side tally artifact, not bad columns — +// the server count and the raw column truth agree. Also seeds past one page so +// the count reflects rows the list endpoint never loads. +func TestIncidentCountsByStatusMatchesRawFilterPostgres(t *testing.T) { + p := newTestPostgres(t) // skips when TEST_POSTGRES_DSN is unset + accessor, ok := p.(storage.SQLAccessor) + if !ok { + t.Fatal("postgres backend must implement storage.SQLAccessor") + } + db := accessor.DB() + + // Seed well past a single page so a bounded page can never see the whole + // set. Deterministic status/origin split via modular arithmetic: + // origin = ai_detect when g%2==0 else webhook + // resolved when g%3==0; else acked when g%5==0; else open + const n = 2500 + if _, err := db.Exec(` + INSERT INTO vs_incidents (id, created_at, origin, resolved, acked_at, resolved_at, title) + SELECT + 'st-' || g, + now() - (g || ' seconds')::interval, + CASE WHEN g % 2 = 0 THEN 'ai_detect' ELSE 'webhook' END, + (g % 3 = 0), + CASE WHEN g % 3 <> 0 AND g % 5 = 0 THEN now() ELSE NULL END, + CASE WHEN g % 3 = 0 THEN now() ELSE NULL END, + 'incident ' || g + FROM generate_series(1, $1) AS g`, n); err != nil { + t.Fatalf("bulk seed: %v", err) + } + + pager := p.(storage.IncidentPager) + got, err := pager.CountIncidentsByStatus() + if err != nil { + t.Fatalf("CountIncidentsByStatus: %v", err) + } + + // Raw truth straight from the stored columns — the FILTER breakdown must + // match it exactly. + rawTotal := func(where string) int { + t.Helper() + var c int + if err := db.QueryRow(`SELECT count(*) FROM vs_incidents WHERE ` + where).Scan(&c); err != nil { + t.Fatalf("raw count (%s): %v", where, err) + } + return c + } + + checks := []struct { + name string + got int + where string + }{ + {"total open", got.Total.Open, "resolved = false AND acked_at IS NULL"}, + {"total acked", got.Total.Acked, "resolved = false AND acked_at IS NOT NULL"}, + {"total resolved", got.Total.Resolved, "resolved = true"}, + {"total all", got.Total.Total, "true"}, + {"ai open", got.AIDetect.Open, "origin = 'ai_detect' AND resolved = false AND acked_at IS NULL"}, + {"ai acked", got.AIDetect.Acked, "origin = 'ai_detect' AND resolved = false AND acked_at IS NOT NULL"}, + {"ai resolved", got.AIDetect.Resolved, "origin = 'ai_detect' AND resolved = true"}, + {"ai all", got.AIDetect.Total, "origin = 'ai_detect'"}, + {"webhook open", got.Webhook.Open, "origin <> 'ai_detect' AND resolved = false AND acked_at IS NULL"}, + {"webhook acked", got.Webhook.Acked, "origin <> 'ai_detect' AND resolved = false AND acked_at IS NOT NULL"}, + {"webhook resolved", got.Webhook.Resolved, "origin <> 'ai_detect' AND resolved = true"}, + {"webhook all", got.Webhook.Total, "origin <> 'ai_detect'"}, + } + for _, c := range checks { + if raw := rawTotal(c.where); c.got != raw { + t.Errorf("%s: count = %d, raw SELECT count(*) FILTER = %d", c.name, c.got, raw) + } + } + + assertStatusInvariants(t, got) +} + +// assertStatusInvariants checks the two reconciliation rules every surface +// depends on: statuses sum to the origin total, and origins sum to the status +// total. +func assertStatusInvariants(t *testing.T, c storage.IncidentStatusCounts) { + t.Helper() + for _, o := range []struct { + name string + s storage.StatusCounts + }{ + {"ai_detect", c.AIDetect}, + {"webhook", c.Webhook}, + {"total", c.Total}, + } { + if o.s.Open+o.s.Acked+o.s.Resolved != o.s.Total { + t.Errorf("%s: open+acked+resolved (%d+%d+%d) != total %d", + o.name, o.s.Open, o.s.Acked, o.s.Resolved, o.s.Total) + } + } + for _, s := range []struct { + name string + ai, webhook, tot int + }{ + {"open", c.AIDetect.Open, c.Webhook.Open, c.Total.Open}, + {"acked", c.AIDetect.Acked, c.Webhook.Acked, c.Total.Acked}, + {"resolved", c.AIDetect.Resolved, c.Webhook.Resolved, c.Total.Resolved}, + {"all", c.AIDetect.Total, c.Webhook.Total, c.Total.Total}, + } { + if s.ai+s.webhook != s.tot { + t.Errorf("%s: ai+webhook (%d+%d) != total %d", s.name, s.ai, s.webhook, s.tot) + } + } +} diff --git a/pkg/storage/memory.go b/pkg/storage/memory.go index 535ff69..c409f6a 100644 --- a/pkg/storage/memory.go +++ b/pkg/storage/memory.go @@ -162,6 +162,17 @@ func (m *memoryProvider) CountIncidents() (IncidentCounts, error) { return c, nil } +// CountIncidentsByStatus implements the optional storage.IncidentPager +// capability. The in-memory history is already capped, so a single pass over +// the slice is cheap; the shared StatusCountsOf helper classifies each row via +// EffectiveOrigin and buckets it by stored status, matching the SQL backend +// exactly. +func (m *memoryProvider) CountIncidentsByStatus() (IncidentStatusCounts, error) { + m.mu.RLock() + defer m.mu.RUnlock() + return StatusCountsOf(m.incidents), nil +} + // ListIncidentsPage implements the optional storage.IncidentPager // capability: one bounded, newest-first page over the in-memory slice, // skipping offset matches and returning at most limit rows. The origin diff --git a/pkg/storage/postgres.go b/pkg/storage/postgres.go index f81dd68..d5092ae 100644 --- a/pkg/storage/postgres.go +++ b/pkg/storage/postgres.go @@ -766,6 +766,37 @@ func (p *postgresProvider) CountIncidents() (IncidentCounts, error) { return c, nil } +// CountIncidentsByStatus implements the optional storage.IncidentPager +// capability: the whole-set per-origin × per-status tally in ONE COUNT query, +// without shipping a single row to Go. The per-status buckets read the +// promoted resolved / acked_at columns and the per-origin split reads the +// promoted origin column, so the whole query is index-friendly. Only the +// whole-set totals and the ai_detect slice are counted in SQL; webhook is +// derived as (total − ai_detect) by AssembleStatusCounts so a legacy row with +// an empty origin — which classifies as webhook via EffectiveOrigin — is +// counted as webhook here too, keeping AIDetect + Webhook == Total. +func (p *postgresProvider) CountIncidentsByStatus() (IncidentStatusCounts, error) { + const q = ` + SELECT + COUNT(*) FILTER (WHERE resolved = false AND acked_at IS NULL) AS open_total, + COUNT(*) FILTER (WHERE resolved = false AND acked_at IS NOT NULL) AS acked_total, + COUNT(*) FILTER (WHERE resolved = true) AS resolved_total, + COUNT(*) AS all_total, + COUNT(*) FILTER (WHERE origin = 'ai_detect' AND resolved = false AND acked_at IS NULL) AS open_ai, + COUNT(*) FILTER (WHERE origin = 'ai_detect' AND resolved = false AND acked_at IS NOT NULL) AS acked_ai, + COUNT(*) FILTER (WHERE origin = 'ai_detect' AND resolved = true) AS resolved_ai, + COUNT(*) FILTER (WHERE origin = 'ai_detect') AS all_ai + FROM vs_incidents` + var total, ai StatusCounts + if err := p.db.QueryRow(q).Scan( + &total.Open, &total.Acked, &total.Resolved, &total.Total, + &ai.Open, &ai.Acked, &ai.Resolved, &ai.Total, + ); err != nil { + return IncidentStatusCounts{}, fmt.Errorf("storage: count incidents by status: %w", err) + } + return AssembleStatusCounts(total, ai), nil +} + // ListIncidentsPage implements the optional storage.IncidentPager // capability: one bounded, newest-first page pushed entirely into SQL // (ORDER BY created_at DESC LIMIT/OFFSET). When origin is one of the known @@ -1026,6 +1057,40 @@ func (p *postgresProvider) CountIncidentsMatching(query string) (IncidentCounts, return c, nil } +// CountIncidentsMatchingByStatus implements the optional +// storage.IncidentSearchPager capability: the per-origin × per-status tally of +// search matches in ONE COUNT query — the search-path twin of +// CountIncidentsByStatus. An empty query degrades to counting every incident, +// matching CountIncidentsByStatus. Like the plain count only the whole-match +// totals and the ai_detect slice are counted; webhook is the complement so +// AIDetect + Webhook == Total holds over the match set too. +func (p *postgresProvider) CountIncidentsMatchingByStatus(query string) (IncidentStatusCounts, error) { + if query == "" { + return p.CountIncidentsByStatus() + } + pattern := "%" + query + "%" + q := fmt.Sprintf(` + SELECT + COUNT(*) FILTER (WHERE resolved = false AND acked_at IS NULL) AS open_total, + COUNT(*) FILTER (WHERE resolved = false AND acked_at IS NOT NULL) AS acked_total, + COUNT(*) FILTER (WHERE resolved = true) AS resolved_total, + COUNT(*) AS all_total, + COUNT(*) FILTER (WHERE origin = 'ai_detect' AND resolved = false AND acked_at IS NULL) AS open_ai, + COUNT(*) FILTER (WHERE origin = 'ai_detect' AND resolved = false AND acked_at IS NOT NULL) AS acked_ai, + COUNT(*) FILTER (WHERE origin = 'ai_detect' AND resolved = true) AS resolved_ai, + COUNT(*) FILTER (WHERE origin = 'ai_detect') AS all_ai + FROM vs_incidents + WHERE (%[1]s)`, searchIncidentsWhereSQL) + var total, ai StatusCounts + if err := p.db.QueryRow(q, pattern).Scan( + &total.Open, &total.Acked, &total.Resolved, &total.Total, + &ai.Open, &ai.Acked, &ai.Resolved, &ai.Total, + ); err != nil { + return IncidentStatusCounts{}, fmt.Errorf("storage: count matching incidents by status: %w", err) + } + return AssembleStatusCounts(total, ai), nil +} + // SearchIncidentsPage implements the optional storage.IncidentSearchPager // capability: one bounded, newest-first page of search matches, with the // query, the origin filter, the ordering, and the LIMIT/OFFSET all pushed diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 0e0350d..db8d730 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -213,6 +213,83 @@ type IncidentCounts struct { Total int } +// StatusCounts is the per-status split of a set of incidents: how many are +// open, acked, and resolved, with Total the sum of the three. The buckets are +// mutually exclusive and match both how the UI labels an incident and how the +// row is stored: +// +// Open = resolved = false AND acked_at IS NULL +// Acked = resolved = false AND acked_at IS NOT NULL +// Resolved = resolved = true +// Total = Open + Acked + Resolved (every row) +type StatusCounts struct { + Open int + Acked int + Resolved int + Total int +} + +// IncidentStatusCounts is the whole-set per-origin × per-status tally the +// count surfaces (the Now page, the header badge, the Incidents page) display. +// Each origin bucket carries its own open/acked/resolved/total and Total is +// the both-origins sum, so AIDetect.X + Webhook.X == Total.X for every status +// X. Like IncidentCounts it is computed WITHOUT materializing rows — one +// COUNT(*) FILTER (…) on SQL backends, a single pass over the capped in-memory +// slice on file/memory — so a large history never has to be loaded to render a +// count. It is the single authoritative source for every number the count +// surfaces show, so those surfaces can never disagree. +type IncidentStatusCounts struct { + AIDetect StatusCounts + Webhook StatusCounts + Total StatusCounts +} + +// StatusCountsOf tallies a materialized set of records into the per-origin × +// per-status breakdown, classifying each row via EffectiveOrigin (so legacy +// empty-origin rows land in webhook) and bucketing by its stored status. The +// file and memory backends use it over their capped in-memory slice, and the +// controllers' fallback path (a backend with no bounded pager) uses it over +// the materialized window, so every path produces the identical shape. +func StatusCountsOf(recs []*IncidentRecord) IncidentStatusCounts { + var out IncidentStatusCounts + for _, rec := range recs { + bucket := &out.Webhook + if rec.EffectiveOrigin() == OriginAIDetect { + bucket = &out.AIDetect + } + switch { + case rec.Resolved: + bucket.Resolved++ + out.Total.Resolved++ + case rec.AckedAt != nil: + bucket.Acked++ + out.Total.Acked++ + default: + bucket.Open++ + out.Total.Open++ + } + bucket.Total++ + out.Total.Total++ + } + return out +} + +// AssembleStatusCounts builds the per-origin × per-status result from the +// whole-set totals and the ai_detect slice, deriving webhook as the complement +// (total − ai_detect) so AIDetect + Webhook == Total for every status +// regardless of how a legacy empty-origin row is classified in SQL. SQL +// backends compute `total` and `ai_detect` with COUNT/FILTER and hand them +// here rather than counting webhook separately. +func AssembleStatusCounts(total, ai StatusCounts) IncidentStatusCounts { + web := StatusCounts{ + Open: total.Open - ai.Open, + Acked: total.Acked - ai.Acked, + Resolved: total.Resolved - ai.Resolved, + Total: total.Total - ai.Total, + } + return IncidentStatusCounts{AIDetect: ai, Webhook: web, Total: total} +} + // IncidentPager is an optional capability a backend may implement on top of // Provider to serve the incident list without ever loading the whole table. // It splits the two things the list endpoint needs — a cheap count and a @@ -230,6 +307,14 @@ type IncidentPager interface { // rows with no explicit Origin are classified from Source exactly as // EffectiveOrigin does, so they are never dropped into an empty bucket. CountIncidents() (IncidentCounts, error) + // CountIncidentsByStatus returns the whole-set per-origin × per-status + // tally (open / acked / resolved / total, each split ai_detect vs webhook), + // computed without materializing rows. It is the authoritative source for + // every count the list surfaces display, so the header badge, the Now page + // and the Incidents page can never disagree. Legacy rows with no explicit + // Origin are classified from Source exactly as EffectiveOrigin does, so + // AIDetect + Webhook always equals Total for each status. + CountIncidentsByStatus() (IncidentStatusCounts, error) // ListIncidentsPage returns one bounded page of incidents, newest first, // skipping the first offset rows and returning at most limit rows. The // page lists ALL incidents (resolved and open alike) so resolved @@ -276,6 +361,13 @@ type IncidentSearchPager interface { // CountIncidents. An empty query counts every unresolved incident (same as // IncidentPager.CountIncidents). CountIncidentsMatching(query string) (IncidentCounts, error) + // CountIncidentsMatchingByStatus returns the per-origin × per-status tally + // of incidents matching query, computed without materializing rows — the + // search-path twin of IncidentPager.CountIncidentsByStatus. An empty query + // counts every incident (same as CountIncidentsByStatus), so the Incidents + // page shows server-authoritative per-status counts over a filtered feed + // too, never a tally of the loaded page. + CountIncidentsMatchingByStatus(query string) (IncidentStatusCounts, error) // SearchIncidentsPage returns one bounded page of incidents matching // query, newest first, filtered to origin (empty = all origins), skipping // offset rows and returning at most limit rows. The page lists ALL diff --git a/src/agent/channels/slack.md b/src/agent/channels/slack.md index 1c0b3c8..d737c2b 100644 --- a/src/agent/channels/slack.md +++ b/src/agent/channels/slack.md @@ -27,6 +27,13 @@ Enable from the environment instead of YAML with `SLACK_ENABLE=true`. 4. Invite the bot to the target channel and copy its **Channel ID** (the `C…` value in the channel details) into `SLACK_CHANNEL_ID`. +> **Reports need channel membership.** Alerts post a message and work as +> long as the bot can post to the channel, but the scheduled/aggregate +> **report** uploads a PNG file, and Slack only lets a bot share a file into +> a channel it has *joined*. Make sure the bot is invited to the channel +> (`/invite @`) or grant it the `channels:join` scope so Versus can +> auto-join public channels before uploading. + ## Full reference ```yaml diff --git a/ui/src/lib/adminUiImprovements.test.ts b/ui/src/lib/adminUiImprovements.test.ts index 73a354b..71bb1bd 100644 --- a/ui/src/lib/adminUiImprovements.test.ts +++ b/ui/src/lib/adminUiImprovements.test.ts @@ -31,11 +31,15 @@ const learnedSignals = read("../pages/LearnedSignalsView.tsx"); const servicesPage = read("../pages/ServicesPage.tsx"); describe("Item 2 — top-bar AI/webhook split + no sidebar count", () => { - it("useOpenIncidentCount tallies open incidents by origin from the shared cache", () => { - expect(hooks.includes("countByOrigin")).toBe(true); - // Same cache key the Now page + badges share — one fetch, no extra load. - expect(/queryKey:\s*\["incidents",\s*"list"\]/.test(hooks)).toBe(true); - expect(/originCounts:\s*countByOrigin\(openList\)/.test(hooks)).toBe(true); + it("useOpenIncidentCount reads the AUTHORITATIVE server counts, not a loaded page", () => { + // The badge must never tally a bounded, loaded array — it reads the cheap + // per-origin × per-status server count endpoint and takes the OPEN slice. + expect(hooks.includes("countByOrigin")).toBe(false); + expect(hooks.includes("api.incidentCounts()")).toBe(true); + // Shared counts cache key the Now page uses too — one fetch, no rows. + expect(/queryKey:\s*\["incidents",\s*"counts"\]/.test(hooks)).toBe(true); + // Open grand total + per-origin open both come from by_status.open. + expect(/by_status\?\.open/.test(hooks)).toBe(true); }); it("the top bar renders the AI/webhook split via formatOriginCounts", () => { diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index 4128be6..3affed3 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -467,6 +467,27 @@ export interface OriginCounts { total: number; } +// IncidentStatusCounts is the whole-set per-origin × per-status breakdown the +// server computes cheaply (COUNT/FILTER on Postgres, one in-memory pass on +// file/memory — never materializing rows). Every NUMBER the count surfaces +// show (the header badge, the Now KPI tiles + origin badges, the Incidents +// status/origin tabs) is read from here, so those surfaces can never disagree. +// Each status bucket is split ai_detect / webhook / total, and +// open+acked+resolved === all per origin. +export interface IncidentStatusCounts { + open: OriginCounts; + acked: OriginCounts; + resolved: OriginCounts; + all: OriginCounts; +} + +// IncidentCounts is the counts object the list / search / counts endpoints +// return: the back-compat top-level unresolved (open-work) per-origin tally, +// plus the authoritative per-origin × per-status breakdown under by_status. +export interface IncidentCounts extends OriginCounts { + by_status?: IncidentStatusCounts; +} + // IncidentIndex is the full list/search response: one bounded, most-recent // page of rows plus the whole-set origin counts computed cheaply on the // server (never by loading every row). `total` is the number of rows matching @@ -476,7 +497,7 @@ export interface OriginCounts { // when this page reached the end). export interface IncidentIndex { incidents: IncidentSummary[]; - counts: OriginCounts; + counts: IncidentCounts; total: number; offset?: number; next_offset?: number | null; @@ -1635,6 +1656,13 @@ export const api = { `/api/admin/incidents${qs}`, ).then((r) => r.incidents ?? []); }, + // incidentCounts fetches the whole-set per-origin × per-status tally WITHOUT + // loading a single row — the cheap COUNT/FILTER endpoint. The header badge + // and the Now page read their numbers from here so they never count a + // bounded, loaded page. The Incidents page reads the same breakdown off its + // list/search response's `counts.by_status` instead of a second request. + incidentCounts: () => + request("/api/admin/incidents/counts"), // listIncidentsIndex is the Incidents-page variant: it returns one bounded, // most-recent page of rows for one origin tab PLUS the whole-set per-origin // counts (so the top-bar shows both feeds separately and the true total) in diff --git a/ui/src/lib/hooks.ts b/ui/src/lib/hooks.ts index 0dea059..caff04a 100644 --- a/ui/src/lib/hooks.ts +++ b/ui/src/lib/hooks.ts @@ -2,7 +2,6 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { useNavigate } from "react-router-dom"; import { useQuery } from "@tanstack/react-query"; import { api } from "./api"; -import { countByOrigin } from "./incidentList"; // --------------------------------------------------------------------------- // useTableKeys — j/k row navigation + Enter to open, for dense tables. @@ -148,22 +147,22 @@ export function useShortcuts({ onHelp }: { onHelp: () => void }) { // --------------------------------------------------------------------------- // useOpenIncidentCount — shared by the TopBar count + the Now page. Polls the -// incident list every 30s, pauses while the tab is hidden. Alongside the plain -// open count it exposes the per-ORIGIN tally of the OPEN incidents (AI-detect -// vs webhook), computed from the SAME ["incidents","list"] cache via the shared -// countByOrigin helper, so the top bar can show the two feeds separately -// ("AI: N · Webhook: M") without a second request. The Sidebar deliberately -// shows NO count. +// cheap server counts endpoint every 30s (never loads incident rows), pausing +// while the tab is hidden. It exposes the OPEN grand total plus the per-ORIGIN +// OPEN tally (AI-detect vs webhook) straight from the server's authoritative +// per-origin × per-status breakdown, so the top bar shows the two feeds +// separately ("AI: N · Webhook: M") without ever counting a bounded, loaded +// page. The Sidebar deliberately shows NO count. // --------------------------------------------------------------------------- export function useOpenIncidentCount() { const q = useQuery({ - queryKey: ["incidents", "list"], - queryFn: () => api.listIncidents(), + queryKey: ["incidents", "counts"], + queryFn: () => api.incidentCounts(), refetchInterval: () => (document.hidden ? false : 30_000), staleTime: 15_000, }); - const openList = (q.data ?? []).filter((i) => !i.resolved && !i.acked_at); - return { open: openList.length, originCounts: countByOrigin(openList), query: q }; + const open = q.data?.by_status?.open; + return { open: open?.total ?? 0, originCounts: open, query: q }; } // --------------------------------------------------------------------------- diff --git a/ui/src/pages/IncidentsPage.test.tsx b/ui/src/pages/IncidentsPage.test.tsx index 1f49373..9400c54 100644 --- a/ui/src/pages/IncidentsPage.test.tsx +++ b/ui/src/pages/IncidentsPage.test.tsx @@ -12,7 +12,13 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { MemoryRouter, Routes, Route, useLocation } from "react-router-dom"; import { ToastProvider } from "@/components/Toast"; import { IncidentsPage } from "./IncidentsPage"; -import { api, type IncidentIndex, type IncidentSummary } from "@/lib/api"; +import { + api, + type IncidentIndex, + type IncidentStatusCounts, + type IncidentSummary, + type OriginCounts, +} from "@/lib/api"; // The Incidents table row exposes ONLY the eye (Assign / Resolve moved to the // bulk-action bar), and the row itself is no longer a navigation control — @@ -49,10 +55,32 @@ function incident(overrides: Partial = {}): IncidentSummary { }; } -function index(rows: IncidentSummary[]): IncidentIndex { +function oc(ai: number, webhook: number): OriginCounts { + return { ai_detect: ai, webhook, total: ai + webhook }; +} + +// index builds a list response. by_status is the server's authoritative +// per-origin × per-status breakdown; when omitted it is derived treating every +// loaded row as an open ai_detect incident (the common single-row fixture). +function index( + rows: IncidentSummary[], + by_status?: IncidentStatusCounts, +): IncidentIndex { + const bs = + by_status ?? { + open: oc(rows.length, 0), + acked: oc(0, 0), + resolved: oc(0, 0), + all: oc(rows.length, 0), + }; return { incidents: rows, - counts: { ai_detect: rows.length, webhook: 0, total: rows.length }, + counts: { + ai_detect: bs.open.ai_detect + bs.acked.ai_detect, + webhook: bs.open.webhook + bs.acked.webhook, + total: bs.open.total + bs.acked.total, + by_status: bs, + }, total: rows.length, }; } @@ -185,3 +213,40 @@ describe("IncidentsPage — webhook auto-resolve toggle", () => { ); }); }); + +// The status- and origin-tab counts must be the SERVER's authoritative +// per-origin × per-status totals — never a tally of the bounded loaded page. +// This is the fix for the "three surfaces, three numbers" bug: with a webhook +// history that auto-resolves, the loaded page holds a single OPEN row yet the +// server sees 277 resolved, so the Resolved tab must read 277 (server), not 0 +// (loaded page), and origin All must read the whole-set 278. +describe("IncidentsPage — tab counts come from server by_status", () => { + it("shows server per-status totals, not the loaded page", async () => { + const loaded = incident({ + id: "wh-open-1", + origin: "webhook", + source: "webhook", + resolved: false, + }); + const byStatus: IncidentStatusCounts = { + open: oc(0, 2), + acked: oc(0, 5), + resolved: oc(0, 277), + all: oc(0, 284), + }; + vi.mocked(api.listIncidentsIndex).mockResolvedValue( + index([loaded], byStatus), + ); + + renderPageAt("/incidents?origin=webhook&status=resolved"); + + // The Resolved status tab shows the server's 277 — the loaded page has zero + // resolved rows, so a client tally would have shown 0. + expect(await screen.findByText("277")).toBeTruthy(); + // The Acked tab shows the server's 5 (also absent from the loaded page). + expect(screen.getByText("5")).toBeTruthy(); + // The webhook feed total (284) reconciles across the origin tab and the + // "All" status tab — the SAME server number in both places. + expect(screen.getAllByText("284").length).toBeGreaterThanOrEqual(2); + }); +}); diff --git a/ui/src/pages/IncidentsPage.tsx b/ui/src/pages/IncidentsPage.tsx index b621655..63671d5 100644 --- a/ui/src/pages/IncidentsPage.tsx +++ b/ui/src/pages/IncidentsPage.tsx @@ -16,7 +16,7 @@ import { Search, UserPlus, } from "lucide-react"; -import { api, type IncidentIndex, type IncidentSummary, type IntakeSettings } from "@/lib/api"; +import { api, type IncidentIndex, type IncidentSummary, type IntakeSettings, type OriginCounts } from "@/lib/api"; import { fmtAbs, fmtRel, incidentTitle, truncate } from "@/lib/format"; import { useTableKeys } from "@/lib/hooks"; import { @@ -230,9 +230,12 @@ export function IncidentsPage() { const { data, isLoading, isError, error, refetch, isRefetching, listQuery } = useIncidentIndex(useServerSearch, trimmed, origin); - // rows for the active origin tab; originCounts is whole-set so the - // top-bar shows both feeds separately regardless of the active tab. - const originCounts = data?.counts; + // Whole-set per-origin totals (all statuses) from the server drive the + // origin-tab badges and the top-bar summary, so both feeds stay visible + // regardless of the active tab. by_status is the authoritative per-origin × + // per-status breakdown the status-tab counts read. + const byStatus = data?.counts?.by_status; + const originCounts = byStatus?.all; // Roster lookups are shared by every row — resolve them once here. const teamsQ = useQuery({ queryKey: ["teams"], queryFn: api.listTeams }); @@ -252,8 +255,9 @@ export function IncidentsPage() { }, [membersQ.data]); const rosterLoading = teamsQ.isLoading || membersQ.isLoading; - // Text filter first (counts per status are computed on this set so the - // segmented-control badges reflect the current search), then status. + // Text filter first (drives the client-side pagination of the VISIBLE rows), + // then status. The COUNTS are server-authoritative (see below) — this filter + // only decides which loaded rows render. const textFiltered = useMemo( // When the server already ran the text search, don't re-filter on text // (it matches fields the client can't see, e.g. payload body). @@ -261,14 +265,30 @@ export function IncidentsPage() { [data?.incidents, q, useServerSearch], ); - const counts = useMemo( - () => ({ + // The text filter runs CLIENT-SIDE only when the backend has no server + // search and a query is present; then the loaded page IS the whole capped + // set (memory/file), so counting the filtered rows is correct. Otherwise + // every count comes from the server's per-origin × per-status breakdown. + const clientFiltering = !useServerSearch && trimmed !== ""; + + const counts = useMemo(() => { + const pick = (c?: OriginCounts) => + origin === "webhook" ? c?.webhook ?? 0 : c?.ai_detect ?? 0; + if (byStatus && !clientFiltering) { + return { + open: pick(byStatus.open), + acked: pick(byStatus.acked), + resolved: pick(byStatus.resolved), + all: pick(byStatus.all), + }; + } + return { open: textFiltered.filter((i) => matchesStatus(i, "open")).length, acked: textFiltered.filter((i) => matchesStatus(i, "acked")).length, resolved: textFiltered.filter((i) => matchesStatus(i, "resolved")).length, - }), - [textFiltered], - ); + all: textFiltered.length, + }; + }, [byStatus, clientFiltering, textFiltered, origin]); const filtered = useMemo( () => textFiltered.filter((i) => matchesStatus(i, status)), @@ -507,7 +527,7 @@ export function IncidentsPage() { label: "Resolved", badge: data ? counts.resolved : undefined, }, - { value: "all", label: "All", badge: data ? textFiltered.length : undefined }, + { value: "all", label: "All", badge: data ? counts.all : undefined }, ]} /> {/* Window-scoped incidents-analytics report — spans both origins, so diff --git a/ui/src/pages/NowPage.tsx b/ui/src/pages/NowPage.tsx index 8d8a6aa..d09feb6 100644 --- a/ui/src/pages/NowPage.tsx +++ b/ui/src/pages/NowPage.tsx @@ -22,6 +22,7 @@ import { ApiError, type AgentConfigView, type IncidentSummary, + type OriginCounts, type Status, } from "@/lib/api"; import { @@ -40,7 +41,6 @@ import { ClickableRow } from "@/components/DataTable"; import { SegmentedControl } from "@/components/SegmentedControl"; import { useNowTick, useTableKeys } from "@/lib/hooks"; import { - countByOrigin, formatOriginCounts, matchesOrigin, normalizeOrigin, @@ -66,15 +66,27 @@ export function NowPage() { // Incidents page. const origin = normalizeOrigin(params.get("origin")); - // Shares ["incidents","list"] with useOpenIncidentCount (TopBar/Sidebar - // badges) so the page and the badges never disagree. 15s auto-refresh, - // paused while the tab is hidden; the TopBar ⟳ is the manual path. + // Shares ["incidents","list"] with the feed elsewhere — the loaded rows back + // the latest-10 feed and the 24h trend sparklines below. Every NUMBER on the + // page, though, comes from the server counts query (below), never this + // bounded page. 15s auto-refresh, paused while the tab is hidden. const incidents = useQuery({ queryKey: ["incidents", "list"], queryFn: () => api.listIncidents(), refetchInterval: () => (document.hidden ? false : 15_000), staleTime: 15_000, }); + // The authoritative per-origin × per-status count — one cheap, rows-free + // request shared with the header badge (useOpenIncidentCount, same key). The + // KPI tiles, the origin-tab badges and the open-banner count all read this, + // so the Now page, the header badge and the Incidents page never disagree. + const countsQ = useQuery({ + queryKey: ["incidents", "counts"], + queryFn: () => api.incidentCounts(), + refetchInterval: () => (document.hidden ? false : 15_000), + staleTime: 15_000, + }); + const byStatus = countsQ.data?.by_status; // Same keys as TopBar's chip queries — one cache entry, zero extra load. const config = useQuery({ queryKey: ["agent-config"], @@ -101,34 +113,36 @@ export function NowPage() { return list; }, [incidents.data]); - // Whole-set per-origin tally (both feeds), computed client-side from the - // one shared ["incidents","list"] cache so the segmented-control badges - // and the top-bar summary show BOTH feeds separately regardless of the - // active tab — the webhook count never lumps into the AI count. - const originCounts = useMemo(() => countByOrigin(sorted), [sorted]); + // Whole-set per-origin totals (all statuses) from the server drive the + // origin-tab badges and the top-bar summary, so both feeds stay visible + // regardless of the active tab — the webhook count never lumps into AI. + const originCounts = byStatus?.all; - // The active tab scopes the whole live view (banner, KPI counts, feed, - // trends) to one origin — split client-side rather than refetching, so - // the TopBar/Sidebar open badge (which needs the whole set) keeps - // sharing this cache. + // The active tab scopes the live view (banner PREVIEW rows, feed, trends) to + // one origin. Rows are split client-side (they share the list cache); the + // COUNTS are read from the server breakdown for the active origin below. const scoped = useMemo( () => sorted.filter((i) => matchesOrigin(i, origin)), [sorted, origin], ); - // open = !resolved && !acked_at; acked = acked_at && !resolved. + // Loaded open rows for the banner PREVIEW list only — the open COUNT shown is + // the server number (counts.open), which may exceed these loaded rows. const openIncidents = useMemo( () => scoped.filter((i) => !i.resolved && !i.acked_at), [scoped], ); - const counts = useMemo( - () => ({ - open: openIncidents.length, - acked: scoped.filter((i) => !i.resolved && !!i.acked_at).length, - resolved: scoped.filter((i) => i.resolved).length, - }), - [scoped, openIncidents], - ); + // KPI + banner numbers for the active origin, straight from the server's + // per-origin × per-status breakdown — never a tally of the loaded page. + const counts = useMemo(() => { + const pick = (c?: OriginCounts) => + origin === "webhook" ? c?.webhook ?? 0 : c?.ai_detect ?? 0; + return { + open: pick(byStatus?.open), + acked: pick(byStatus?.acked), + resolved: pick(byStatus?.resolved), + }; + }, [byStatus, origin]); const feed = useMemo(() => scoped.slice(0, 10), [scoped]); // Most recently resolved incident — context for the all-clear banner. @@ -174,9 +188,13 @@ export function NowPage() { }); const refreshing = - incidents.isFetching || status.isFetching || config.isFetching; + incidents.isFetching || + countsQ.isFetching || + status.isFetching || + config.isFetching; const refreshAll = () => { incidents.refetch(); + countsQ.refetch(); status.refetch(); config.refetch(); }; @@ -212,7 +230,7 @@ export function NowPage() { - {/* (1) Open-incident banner — recency-sorted until backend ask #1 - ships severity on summaries; whole card opens the incident. */} - {incidents.isPending && ( + {/* (1) Open-incident banner — the OPEN count is the server number; + the preview rows are the loaded page. Recency-sorted until backend + ask #1 ships severity on summaries. */} + {(incidents.isPending || countsQ.isPending) && (
)} - {incidents.isError && ( + {(incidents.isError || countsQ.isError) && ( incidents.refetch()} - retrying={incidents.isFetching} + error={incidents.error ?? countsQ.error} + onRetry={() => { + incidents.refetch(); + countsQ.refetch(); + }} + retrying={incidents.isFetching || countsQ.isFetching} /> )} - {incidents.isSuccess && openIncidents.length === 0 && ( + {byStatus && counts.open === 0 && (
@@ -288,7 +310,7 @@ export function NowPage() {
)} - {incidents.isSuccess && openIncidents.length > 0 && ( + {byStatus && counts.open > 0 && (
@@ -340,12 +362,12 @@ export function NowPage() {
0 ? "critical" : "ok" @@ -359,19 +381,19 @@ export function NowPage() { /> 0 ? "warn" : undefined} + tone={byStatus && counts.acked > 0 ? "warn" : undefined} spark={incidents.data ? trends.acked : undefined} sparkLabel={`${trends.acked24} incidents acknowledged in the last 24 hours`} foot={incidents.data ? `${trends.acked24} acked · 24h` : undefined} />