feat: single source of truth for count - fix report channel

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) {

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 @<bot>\") 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 {

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)