diff --git a/pkg/channels/channelkinds/slack/blocks_degrade.go b/pkg/channels/channelkinds/slack/blocks_degrade.go index 80438a39..3f8fe60b 100644 --- a/pkg/channels/channelkinds/slack/blocks_degrade.go +++ b/pkg/channels/channelkinds/slack/blocks_degrade.go @@ -36,3 +36,18 @@ func isUnsupportedBlocksErr(err error) bool { return false } } + +// blocksRejectionDetail returns Slack's per-block explanation for a rejected +// Block Kit payload. Slack puts the offending field(s) in +// ResponseMetadata.Messages (typically a json-pointer into /blocks), but +// SlackErrorResponse.Error() surfaces only the bare code ("invalid_blocks"), +// so without this the specific reason is lost at the degrade sites. Returns nil +// when err is not a typed Slack error or carried no messages — a degrade log +// then simply omits the detail rather than inventing one. +func blocksRejectionDetail(err error) []string { + var sre slackapi.SlackErrorResponse + if !errors.As(err, &sre) { + return nil + } + return sre.ResponseMetadata.Messages +} diff --git a/pkg/channels/channelkinds/slack/blocks_degrade_test.go b/pkg/channels/channelkinds/slack/blocks_degrade_test.go new file mode 100644 index 00000000..6fe9dba8 --- /dev/null +++ b/pkg/channels/channelkinds/slack/blocks_degrade_test.go @@ -0,0 +1,20 @@ +package slack + +import ( + "errors" + "testing" + + slackapi "github.com/slack-go/slack" + "github.com/stretchr/testify/assert" +) + +func TestBlocksRejectionDetail(t *testing.T) { + sre := slackapi.SlackErrorResponse{Err: "invalid_blocks"} + sre.ResponseMetadata.Messages = []string{"invalid block [json-pointer:/blocks/0/chart]: too many data points"} + + // Error() surfaces only "invalid_blocks"; the actionable per-block reason + // lives in ResponseMetadata.Messages and would otherwise be dropped. + assert.Equal(t, sre.ResponseMetadata.Messages, blocksRejectionDetail(sre)) + assert.Nil(t, blocksRejectionDetail(errors.New("network down")), "a non-Slack error carries no block detail") + assert.Nil(t, blocksRejectionDetail(nil)) +} diff --git a/pkg/channels/channelkinds/slack/components.go b/pkg/channels/channelkinds/slack/components.go index b3b70a66..1c82aeda 100644 --- a/pkg/channels/channelkinds/slack/components.go +++ b/pkg/channels/channelkinds/slack/components.go @@ -55,8 +55,9 @@ const componentFormattingInstructions = `This channel can render structured Bloc `"markdown" (a standard-markdown block, ≤12000 chars); ` + `"table" (rows of rich_text cells; one table per message, ≤100 rows, ≤20 columns); ` + `"data_visualization" (a chart; REQUIRES a "title" ≤50 chars and a "chart" object. ` + - `pie: {"type":"pie","segments":[{"label":"Organic","value":60}]}. ` + - `bar/area/line: {"type":"bar","series":[{"name":"Signups","data":[{"label":"Mon","value":10}]}],"axis_config":{"categories":["Mon","Tue"]}} — note each series uses "data" (not "data_points") and axis_config.categories is required; ≤2 charts per message); ` + + `pie: {"type":"pie","segments":[{"label":"Organic","value":60}]} — 1–6 segments. ` + + `bar/area/line: {"type":"bar","series":[{"name":"Signups","data":[{"label":"Mon","value":10}]}],"axis_config":{"categories":["Mon","Tue"]}} — each series uses "data" (not "data_points") and axis_config.categories is required. ` + + `Slack caps every chart: 1–6 series; 1–20 x-axis categories, all distinct; EXACTLY one data point per category per series, and each point's "label" must be the identical string as its category (not a reformatted date); bucket long time ranges into ≤20 points — do not emit a point per minute; all category labels, series names, and point labels 1–20 chars; pie segment values must be > 0 (omit zero buckets); ≤2 charts per message); ` + `"plan" and "task_card" (task lists); ` + `"card" (title/subtitle/body/subtext — NO action buttons, NO images yet); ` + `"container" (groups 1–10 read-only child blocks in "child_blocks"; REQUIRES a "title" or "rich_text_title"). ` + @@ -329,11 +330,14 @@ func validateTable(loc string, t *slackapi.TableBlock) error { } // validateDataViz enforces the data_visualization schema Slack's chat.postMessage -// requires (verified empirically against the live API): a non-empty title ≤50 -// chars, a chart of a known type, and the per-type data shape — pie needs -// segments, bar/area/line need series plus axis_config.categories. Enforcing it -// here turns a would-be post-time invalid_blocks (which degrades the whole reply -// to plain text) into a precise error the agent can correct. +// requires by delegating to slack-go's DataVisualizationBlock.Validate, which +// models Slack's per-chart rules (verified against the live API): 1–6 series or +// pie segments, 1–20 unique categories, exactly one point per category whose +// label matches it, 1–20-char labels, pie values > 0, and the title/axis-label +// caps. Enforcing it here turns a would-be post-time invalid_blocks (which +// degrades the whole reply to plain text, sender.go) into a located error the +// agent can correct. The only rule kept locally is a whitespace-only title, +// which Validate's length check lets through. // // It also runs the no-mass-notification guard over EVERY free-form chart string // — the title plus segment labels, series names, data-point labels, axis @@ -345,60 +349,54 @@ func validateDataViz(loc string, dv *slackapi.DataVisualizationBlock) error { if strings.TrimSpace(dv.Title) == "" { return blockErrf(loc, "data_visualization", "requires a non-empty \"title\" (≤%d chars)", maxDataVizTitleLen) } - if n := runeLen(dv.Title); n > maxDataVizTitleLen { - return blockErrf(loc, "data_visualization", "title is %d characters; Slack limits it to %d", n, maxDataVizTitleLen) + if err := dv.Validate(); err != nil { + return blockErrf(loc, "data_visualization", "%v. %s", err, dataVizLimitsHint) } if err := checkBroadcast(loc, "data_visualization", dv.Title); err != nil { return err } + var labels []string switch c := dv.Chart.(type) { case *slackapi.DataVisualizationPieChart: - if len(c.Segments) == 0 { - return blockErrf(loc, "data_visualization", "pie chart requires a non-empty \"segments\" array of {label,value}") - } for _, seg := range c.Segments { - if err := checkBroadcast(loc, "data_visualization", seg.Label); err != nil { - return err - } + labels = append(labels, seg.Label) } case *slackapi.DataVisualizationBarChart: - return validateVizSeries(loc, "bar", c.Series, c.AxisConfig) + labels = vizSeriesStrings(c.Series, c.AxisConfig) case *slackapi.DataVisualizationAreaChart: - return validateVizSeries(loc, "area", c.Series, c.AxisConfig) + labels = vizSeriesStrings(c.Series, c.AxisConfig) case *slackapi.DataVisualizationLineChart: - return validateVizSeries(loc, "line", c.Series, c.AxisConfig) + labels = vizSeriesStrings(c.Series, c.AxisConfig) default: return blockErrf(loc, "data_visualization", "requires a \"chart\" object whose \"type\" is one of pie, bar, area, line") } + for _, l := range labels { + if err := checkBroadcast(loc, "data_visualization", l); err != nil { + return err + } + } return nil } -func validateVizSeries(loc, kind string, series []slackapi.DataVisualizationDataSeries, axis slackapi.DataVisualizationAxisConfig) error { - if len(series) == 0 { - return blockErrf(loc, "data_visualization", "%s chart requires a non-empty \"series\" array; each series is {\"name\":…,\"data\":[{\"label\":…,\"value\":…}]}", kind) - } - if len(axis.Categories) == 0 { - return blockErrf(loc, "data_visualization", "%s chart requires \"axis_config\":{\"categories\":[…]} naming the x-axis categories", kind) - } +// dataVizLimitsHint is appended to a Validate failure so the model gets the +// whole rule set, not just the first rule its chart broke. +const dataVizLimitsHint = `Slack chart rules: 1–6 series (pie: 1–6 segments, each value > 0); ` + + `1–20 unique axis_config.categories (bucket long time ranges into ≤20 points); ` + + `each series has exactly one point per category, and each point's label must equal its category string; ` + + `every category, series name, and label is 1–20 characters` + +// vizSeriesStrings collects the free-form strings of a bar/area/line chart for +// the broadcast guard. +func vizSeriesStrings(series []slackapi.DataVisualizationDataSeries, axis slackapi.DataVisualizationAxisConfig) []string { + out := make([]string, 0, len(axis.Categories)+2) for _, s := range series { - if err := checkBroadcast(loc, "data_visualization", s.Name); err != nil { - return err - } + out = append(out, s.Name) for _, p := range s.Data { - if err := checkBroadcast(loc, "data_visualization", p.Label); err != nil { - return err - } + out = append(out, p.Label) } } - for _, cat := range axis.Categories { - if err := checkBroadcast(loc, "data_visualization", cat); err != nil { - return err - } - } - if err := checkBroadcast(loc, "data_visualization", axis.XLabel); err != nil { - return err - } - return checkBroadcast(loc, "data_visualization", axis.YLabel) + out = append(out, axis.Categories...) + return append(out, axis.XLabel, axis.YLabel) } func validateContainer(loc string, c *slackapi.ContainerBlock) error { diff --git a/pkg/channels/channelkinds/slack/components_test.go b/pkg/channels/channelkinds/slack/components_test.go index 65e7ce1a..8f1e06d7 100644 --- a/pkg/channels/channelkinds/slack/components_test.go +++ b/pkg/channels/channelkinds/slack/components_test.go @@ -1,6 +1,7 @@ package slack import ( + "fmt" "strings" "testing" @@ -8,6 +9,46 @@ import ( "github.com/stretchr/testify/require" ) +// vizLineChart builds a line-chart data_visualization block with `points` +// x-axis categories and one series carrying exactly one aligned point each — +// the shape a time-series answer produces. Used to exercise Slack's 1–20 +// category/point cap. +func vizLineChart(points int) string { + cats := make([]string, points) + data := make([]string, points) + for i := range cats { + cats[i] = fmt.Sprintf(`"t%d"`, i) + data[i] = fmt.Sprintf(`{"label":"t%d","value":%d}`, i, i) + } + return `[{"type":"data_visualization","title":"p95","chart":{"type":"line",` + + `"series":[{"name":"p95","data":[` + strings.Join(data, ",") + `]}],` + + `"axis_config":{"categories":[` + strings.Join(cats, ",") + `]}}}]` +} + +// vizManySeries builds a bar chart with `n` single-point series over one +// category, to exercise Slack's 1–6 series-per-chart cap. +func vizManySeries(n int) string { + series := make([]string, n) + for i := range series { + series[i] = fmt.Sprintf(`{"name":"s%d","data":[{"label":"Mon","value":%d}]}`, i, i) + } + return `[{"type":"data_visualization","title":"x","chart":{"type":"bar",` + + `"series":[` + strings.Join(series, ",") + `],` + + `"axis_config":{"categories":["Mon"]}}}]` +} + +// vizBar builds a bar chart from a raw categories array and raw series +// objects, so each case states only the part of the chart it is breaking. +func vizBar(categories, series string) string { + return `[{"type":"data_visualization","title":"x","chart":{"type":"bar",` + + `"series":[` + series + `],"axis_config":{"categories":` + categories + `}}}]` +} + +// vizPie builds a pie chart from raw segment objects. +func vizPie(segments string) string { + return `[{"type":"data_visualization","title":"x","chart":{"type":"pie","segments":[` + segments + `]}}]` +} + func repeatBlock(block string, n int) string { parts := make([]string, n) for i := range parts { @@ -166,7 +207,7 @@ func TestValidateComponents(t *testing.T) { {name: "rich_text usergroup element: rejected usergroup ping", json: `[{"type":"rich_text","elements":[{"type":"rich_text_section","elements":[{"type":"usergroup","usergroup_id":"S12345"}]}]}]`, wantErr: "usergroup"}, {name: "data_viz pie segment label : rejected broadcast", json: `[{"type":"data_visualization","title":"Share","chart":{"type":"pie","segments":[{"label":"","value":60}]}}]`, wantErr: "broadcast"}, {name: "data_viz bar series name : rejected usergroup ping", json: `[{"type":"data_visualization","title":"Signups","chart":{"type":"bar","series":[{"name":"","data":[{"label":"Mon","value":10}]}],"axis_config":{"categories":["Mon"]}}}]`, wantErr: "usergroup"}, - {name: "data_viz axis category : rejected broadcast", json: `[{"type":"data_visualization","title":"Signups","chart":{"type":"line","series":[{"name":"S","data":[{"label":"Mon","value":10}]}],"axis_config":{"categories":[""]}}}]`, wantErr: "broadcast"}, + {name: "data_viz axis category : rejected broadcast", json: `[{"type":"data_visualization","title":"Signups","chart":{"type":"line","series":[{"name":"S","data":[{"label":"","value":10}]}],"axis_config":{"categories":[""]}}}]`, wantErr: "broadcast"}, // --- one block slot is reserved for the sender's mention_only footer --- {name: "49 blocks: accepted at the agent cap", json: repeatBlock(`{"type":"divider"}`, 49), wantErr: ""}, @@ -187,6 +228,30 @@ func TestValidateComponents(t *testing.T) { {name: "table of 101 rows: rejected", json: `[` + rawTextTable(101, 1) + `]`, wantErr: "101 rows"}, {name: "table row of 21 cells: rejected", json: `[` + rawTextTable(1, 21) + `]`, wantErr: "21 cells"}, + // --- data_visualization per-chart limits (Slack caps; slack-go + // block_data_visualization.go documents 1–6 series/segments, 1–20 + // categories with exactly one point per category, ≤20-char labels) --- + {name: "line chart with 20 points: accepted at the cap", json: vizLineChart(20), wantErr: ""}, + {name: "line chart with 21 points: rejected over cap", json: vizLineChart(21), wantErr: "cannot have more than 20 points"}, + {name: "bar chart with 6 series: accepted at the cap", json: vizManySeries(6), wantErr: ""}, + {name: "bar chart with 7 series: rejected over cap", json: vizManySeries(7), wantErr: "cannot have more than 6 series"}, + {name: "series point count != categories: rejected", json: vizBar(`["Mon"]`, `{"name":"S","data":[{"label":"Mon","value":1},{"label":"Tue","value":2}]}`), wantErr: "exactly one point for every category"}, + {name: "point label not matching its category: rejected", json: vizBar(`["2026-10-01","2026-10-02"]`, `{"name":"S","data":[{"label":"Oct 1","value":1},{"label":"Oct 2","value":2}]}`), wantErr: "must match axis_config.categories"}, + {name: "repeated point label within a series: rejected", json: vizBar(`["Mon","Tue"]`, `{"name":"S","data":[{"label":"Mon","value":1},{"label":"Mon","value":2}]}`), wantErr: "duplicate label"}, + {name: "repeated category: rejected", json: vizBar(`["Mon","Mon"]`, `{"name":"S","data":[{"label":"Mon","value":1},{"label":"Mon","value":2}]}`), wantErr: "duplicate category"}, + {name: "empty category: rejected", json: vizBar(`[""]`, `{"name":"S","data":[{"label":"","value":1}]}`), wantErr: "categories[0] must have a minimum length of 1"}, + {name: "category label over 20 chars: rejected", json: vizBar(`["`+strings.Repeat("c", 21)+`"]`, `{"name":"S","data":[{"label":"`+strings.Repeat("c", 21)+`","value":1}]}`), wantErr: "categories[0] cannot be longer than 20"}, + {name: "empty series name: rejected", json: vizBar(`["Mon"]`, `{"name":"","data":[{"label":"Mon","value":1}]}`), wantErr: "name must have a minimum length of 1"}, + {name: "series name over 20 chars: rejected", json: vizBar(`["Mon"]`, `{"name":"`+strings.Repeat("s", 21)+`","data":[{"label":"Mon","value":1}]}`), wantErr: "name cannot be longer than 20"}, + {name: "empty data point label: rejected", json: vizBar(`["Mon"]`, `{"name":"S","data":[{"label":"","value":1}]}`), wantErr: "label must have a minimum length of 1"}, + {name: "duplicate series names: rejected", json: vizBar(`["Mon"]`, `{"name":"S","data":[{"label":"Mon","value":1}]},{"name":"S","data":[{"label":"Mon","value":2}]}`), wantErr: "series names must be unique"}, + {name: "pie chart with 7 segments: rejected over cap", json: `[{"type":"data_visualization","title":"x","chart":{"type":"pie","segments":[{"label":"a","value":1},{"label":"b","value":1},{"label":"c","value":1},{"label":"d","value":1},{"label":"e","value":1},{"label":"f","value":1},{"label":"g","value":1}]}}]`, wantErr: "more than 6 segments"}, + {name: "pie segment label over 20 chars: rejected", json: vizPie(`{"label":"` + strings.Repeat("a", 21) + `","value":1}`), wantErr: "label cannot be longer than 20"}, + {name: "pie segment with empty label: rejected", json: vizPie(`{"label":"","value":1}`), wantErr: "label must have a minimum length of 1"}, + {name: "pie segment with zero value: rejected", json: vizPie(`{"label":"Sales","value":5},{"label":"Refunds","value":0}`), wantErr: "value must be greater than 0"}, + {name: "pie segment with negative value: rejected", json: vizPie(`{"label":"Refunds","value":-1}`), wantErr: "value must be greater than 0"}, + {name: "chart rejection: carries the full rule set for the model", json: vizLineChart(21), wantErr: "bucket long time ranges"}, + // --- structurally empty blocks Slack rejects --- {name: "header without text: rejected", json: `[{"type":"header"}]`, wantErr: "header"}, {name: "header with blank text: rejected", json: `[{"type":"header","text":{"type":"plain_text","text":" "}}]`, wantErr: "non-empty"}, diff --git a/pkg/channels/channelkinds/slack/sender.go b/pkg/channels/channelkinds/slack/sender.go index 52f81952..4349b7b1 100644 --- a/pkg/channels/channelkinds/slack/sender.go +++ b/pkg/channels/channelkinds/slack/sender.go @@ -343,7 +343,7 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e // reply: retry the bubble update as plain text so the user still // sees the answer — degraded, logged. logger.Info("slack: streaming-bubble blocks rejected; retrying chat.update as plain text (degraded)", - "session", sess.Name, "channelID", channelID, "bubbleTS", ts, "err", err.Error()) + "session", sess.Name, "channelID", channelID, "bubbleTS", ts, "err", err.Error(), "slackDetail", blocksRejectionDetail(err)) _, _, _, err = s.client.UpdateMessageContext(ctx, channelID, ts, slackapi.MsgOptionText(responseReadyEmoji+" "+degradedPlainText(replyText, pl.Components, sess.Channel.RoutingMode), false)) } @@ -420,7 +420,7 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e // (no Show-settings button), but delivered. Logged loudly so the // degraded path is visible in operator logs. logger.Info("slack: user_message blocks rejected; retrying as plain text (degraded, no Show-settings button)", - "session", sess.Name, "channelID", channelID, "err", err.Error()) + "session", sess.Name, "channelID", channelID, "err", err.Error(), "slackDetail", blocksRejectionDetail(err)) usedBlocks = false plainOpts := []slackapi.MsgOption{slackapi.MsgOptionText(degradedPlainText(replyText, pl.Components, sess.Channel.RoutingMode), false)} if threadTS != "" { diff --git a/pkg/channels/channelkinds/slack/sender_plan.go b/pkg/channels/channelkinds/slack/sender_plan.go index 19ea48ff..415357a3 100644 --- a/pkg/channels/channelkinds/slack/sender_plan.go +++ b/pkg/channels/channelkinds/slack/sender_plan.go @@ -249,7 +249,7 @@ func (s *slackSender) sendPlanUpdate( newTS, wasPost, err := postOrUpdate(renderPlanBlocks(pl)) if err != nil && isUnsupportedBlocksErr(err) { logger.Info("slack: plan/task_card blocks rejected; falling back to legacy emoji rendering", - "session", sess.Name, "err", err.Error()) + "session", sess.Name, "err", err.Error(), "slackDetail", blocksRejectionDetail(err)) newTS, wasPost, err = postOrUpdate(renderPlanBlocksLegacy(pl)) } if err != nil {