diff --git a/go.mod b/go.mod index 040a14bb..030cb501 100644 --- a/go.mod +++ b/go.mod @@ -50,9 +50,9 @@ require ( github.com/ory/dockertest/v3 v3.12.0 github.com/rs/zerolog v1.34.0 github.com/sigstore/sigstore v1.10.8 - github.com/slack-go/slack v0.23.1 + github.com/slack-go/slack v0.30.1 github.com/spf13/cobra v1.10.2 - github.com/stretchr/testify v1.12.0 + github.com/stretchr/testify v1.12.1 github.com/tdewolff/parse/v2 v2.8.13 github.com/tsawler/tabula v1.6.14 github.com/warpstreamlabs/bento v1.17.0 diff --git a/go.sum b/go.sum index 4cd4ac3e..9fc38f76 100644 --- a/go.sum +++ b/go.sum @@ -860,8 +860,8 @@ github.com/sirupsen/logrus v1.10.1 h1:xi4336Zh11WpU14fXR6I67V3yaTPQYwRx2WEtHbRg4 github.com/sirupsen/logrus v1.10.1/go.mod h1:vsQHnG7xzNsxk3NrwboUiWPnIC3dmbjcGPykD7+tiHk= github.com/skeema/knownhosts v1.3.1 h1:X2osQ+RAjK76shCbvhHHHVl3ZlgDm8apHEHFqRjnBY8= github.com/skeema/knownhosts v1.3.1/go.mod h1:r7KTdC8l4uxWRyK2TpQZ/1o5HaSzh06ePQNxPwTcfiY= -github.com/slack-go/slack v0.23.1 h1:ZS5B96wxxYQRwvJ3/vJFtqtUZi3tXhsZCyT44Nv7M80= -github.com/slack-go/slack v0.23.1/go.mod h1:H0yR/YBuRJ39RkE+JpV/d/oEsbanzTRowR82bCN0cEs= +github.com/slack-go/slack v0.30.1 h1:q6FjZssqts6hJ+DRWCnnO2YVUh+E3/UNrC0EIO2xci8= +github.com/slack-go/slack v0.30.1/go.mod h1:radSmEe8GEgE+aK980ZafMR0AQhiZjADptN5k/gOw34= github.com/snowflakedb/gosnowflake v1.18.1 h1:Nb4AWSnSBWe1UKpKTwCZxjYhYo1JH7GgKhO3wW1kR10= github.com/snowflakedb/gosnowflake v1.18.1/go.mod h1:7D4+cLepOWrerVsH+tevW3zdMJ5/WrEN7ZceAC6xBv0= github.com/spf13/afero v1.15.0 h1:b/YBCLWAJdFWJTN9cLhiXXcD7mzKn9Dm86dNnfyQw1I= @@ -893,8 +893,8 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -github.com/stretchr/testify v1.12.0 h1:K6Mr6jO9JICuend/5xzTM03ydSV3vdNRYAdPSukj8uI= -github.com/stretchr/testify v1.12.0/go.mod h1:bOYBZb5qJ00vPzWfIqBUZPaxK8jWiXc6d3ErP4Ca9Gw= +github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= +github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= github.com/subosito/gotenv v1.6.0 h1:9NlTDc1FTs4qu0DDq7AEtTPNw6SVm7uBMsUCUjABIf8= github.com/subosito/gotenv v1.6.0/go.mod h1:Dk4QP5c2W3ibzajGcXpNraDfq2IrhjMIvMSWPKKo0FU= github.com/tdewolff/parse/v2 v2.8.13 h1:si/8rLw5BZZTWCCiMm9A3f6x+RmqYfrkEeXCgpX5ick= diff --git a/pkg/agent/runner/leakageread_meta.go b/pkg/agent/runner/leakageread_meta.go index 0f167c55..b49e20a9 100644 --- a/pkg/agent/runner/leakageread_meta.go +++ b/pkg/agent/runner/leakageread_meta.go @@ -215,7 +215,7 @@ func (l *Loop) readSidePostForMetaTool( Tool: &pipeline.ToolCallInfo{ Name: name, // The bytes the tool was actually called with. The gated path passes - // its stripArgTags'd copy because it strips before executing; this + // its toolenvelope.StripPtJSON'd copy because it strips before executing; this // path does not strip at all, so the same copy the tool saw is the // honest answer. No plural declaration reads Args anyway — a // resource named by an ARG is knowable before the call and takes the diff --git a/pkg/agent/runner/loop_dispatch.go b/pkg/agent/runner/loop_dispatch.go index 580bf4ca..1804930b 100644 --- a/pkg/agent/runner/loop_dispatch.go +++ b/pkg/agent/runner/loop_dispatch.go @@ -15,6 +15,7 @@ import ( lifecyclecore "github.com/authzed/openagentprimitives/pkg/agent/session/lifecycle" "github.com/authzed/openagentprimitives/pkg/agent/tool" "github.com/authzed/openagentprimitives/pkg/agent/tool/sandbox" + "github.com/authzed/openagentprimitives/pkg/agent/toolenvelope" "github.com/authzed/openagentprimitives/pkg/authz/toolguard" "github.com/authzed/openagentprimitives/pkg/platform/identity" "github.com/authzed/openagentprimitives/pkg/platform/identity/credupdate" @@ -441,7 +442,7 @@ func (l *Loop) executeToolContained( // The tool must not see pt markup: strip any pt-untrusted envelope from the // args before the tool runs. Inert (args unchanged) when the args carry no // envelope. - args = stripArgTags(args) + args, _ = toolenvelope.StripPtJSON(args) // Re-check revocation IMMEDIATELY before the call runs. // diff --git a/pkg/agent/runner/pttag_argstrip.go b/pkg/agent/runner/pttag_argstrip.go deleted file mode 100644 index d8ddc05b..00000000 --- a/pkg/agent/runner/pttag_argstrip.go +++ /dev/null @@ -1,63 +0,0 @@ -package runner - -import ( - "bytes" - "encoding/json" - - "github.com/authzed/openagentprimitives/pkg/agent/toolenvelope" -) - -// stripArgTags removes pt-untrusted envelope markers from the args a TOOL will -// see. Tools do not know about pt markup; this is where it stops, between the -// gates and the tool's own Execute. -// -// Args are JSON and the markup rides inside string VALUES (quotes escaped in the -// raw bytes), so it walks the decoded string leaves. Numbers are decoded with -// UseNumber so a large-integer sibling of a tagged field (a Snowflake id, a -// nanosecond timestamp) survives the round-trip intact. On any parse failure, or -// when nothing was stripped, it returns the ORIGINAL bytes unchanged — the tool -// then sees exactly what the model sent, which is the safe fallback. -func stripArgTags(args json.RawMessage) json.RawMessage { - if len(args) == 0 { - return args - } - dec := json.NewDecoder(bytes.NewReader(args)) - dec.UseNumber() - var v any - if err := dec.Decode(&v); err != nil { - return args - } - had := false - walked := walkStripEnvelopes(v, &had) - if !had { - return args // nothing tagged; leave the exact bytes (no needless re-marshal) - } - out, err := json.Marshal(walked) - if err != nil { - return args - } - return json.RawMessage(out) -} - -func walkStripEnvelopes(v any, had *bool) any { - switch t := v.(type) { - case string: - s, found := toolenvelope.StripPt(t) - if found { - *had = true - } - return s - case []any: - for i, e := range t { - t[i] = walkStripEnvelopes(e, had) - } - return t - case map[string]any: - for k, e := range t { - t[k] = walkStripEnvelopes(e, had) - } - return t - default: - return v - } -} diff --git a/pkg/agent/tool/meta/respond.go b/pkg/agent/tool/meta/respond.go index cef46960..35c5aa17 100644 --- a/pkg/agent/tool/meta/respond.go +++ b/pkg/agent/tool/meta/respond.go @@ -88,10 +88,16 @@ func New(cfg RespondConfig) tool.Tool { } type respondTool struct { - cfg RespondConfig - schema json.RawMessage - desc string - hasAsset bool + cfg RespondConfig + schema json.RawMessage + desc string + hasAsset bool + hasComponents bool + // components validates and projects agent-authored `blocks`. Resolved from + // the outbound kind at construction; nil when the kind advertises no + // "components" capability, or (a wiring bug) advertises it without a + // validator — Execute fails closed on the latter. + components channelkinds.ComponentValidator } func newRespondTool(cfg RespondConfig) *respondTool { @@ -109,27 +115,33 @@ func newRespondTool(cfg RespondConfig) *respondTool { } } + // WHAT a reply may carry and HOW it is authored are the KIND's facts, not + // this tool's: the rules land in model-facing prompt text the runner + // presents as channel-specific and authoritative, so asking the registry + // keeps that promise for every registered kind. An unregistered name yields + // a nil Kind — TextFormattingInstructionsFor answers with the generic + // CommonMark line, and ComponentValidatorFor answers with nil. + kind, _ := chregistry.Get(cfg.ChannelKind) + props := map[string]any{} required := []string{"text"} textDesc := "The message text." if hasMarkdown { - // WHICH markup dialect this surface speaks is the KIND's fact, not this - // tool's: the rules land in model-facing prompt text the runner presents - // as channel-specific and authoritative, so asking the registry keeps - // that promise for every registered kind. An unregistered name yields a - // nil Kind, which TextFormattingInstructionsFor answers with the generic - // CommonMark line. - kind, _ := chregistry.Get(cfg.ChannelKind) textDesc += " " + channelkinds.TextFormattingInstructionsFor(kind) } else { textDesc += " Plain text only — markdown will be sent literally." } props["text"] = map[string]any{"type": "string", "description": textDesc} + var components channelkinds.ComponentValidator if hasComponents { + components = channelkinds.ComponentValidatorFor(kind) + desc := "Optional structured UI components in the channel-kind's native format, rendered as the message body with `text` as the notification preview and fallback. " + + channelkinds.ComponentFormattingInstructionsFor(kind) props["blocks"] = map[string]any{ "type": "array", - "description": "Optional structured components in the channel-kind's native format.", + "description": desc, + "items": map[string]any{"type": "object"}, } } @@ -156,6 +168,9 @@ func newRespondTool(cfg RespondConfig) *respondTool { } else { descParts = " Plain text only on this channel." } + if hasComponents { + descParts += " Structured `blocks` components are supported." + } desc := fmt.Sprintf( "Send a reply to the user via the %s channel.%s Call this whenever you have a message to surface. "+ "After your reply is complete, call await_user_message to yield until the user responds, "+ @@ -163,7 +178,7 @@ func newRespondTool(cfg RespondConfig) *respondTool { cfg.ChannelKind, descParts, ) - return &respondTool{cfg: cfg, schema: schemaBytes, desc: desc, hasAsset: hasAsset} + return &respondTool{cfg: cfg, schema: schemaBytes, desc: desc, hasAsset: hasAsset, hasComponents: hasComponents, components: components} } func (t *respondTool) Name() string { return "respond_to_user" } @@ -194,14 +209,59 @@ func (t *respondTool) Execute(ctx context.Context, args json.RawMessage, sess *t // Attached are artifact handles to attach; each is gated on the channel's // asset:* capability. Attached []string `json:"attached,omitempty"` + // Blocks are agent-authored structured components in the channel-kind's + // native format, gated on the channel's "components" capability and + // validated by the kind before publish. + Blocks json.RawMessage `json:"blocks,omitempty"` } if res, ok := tool.ParseArgs(args, &in, t.Name(), `{"text": "the message body"}`); !ok { return res, nil } + // Models routinely send an empty optional array ("blocks": [] or null); the + // raw bytes "[]"/"null" are non-empty, so a later len(in.Blocks) > 0 would + // treat them as a present-but-invalid payload and error an otherwise-fine + // plain-text reply (or reject on a non-components channel). Normalize "none" + // to absent so every downstream len()>0 check agrees it isn't there. + in.Blocks = normalizeOptionalBlocks(in.Blocks) if strings.TrimSpace(in.Text) == "" { return tool.Result{Content: "respond_to_user: `text` is required and must be non-empty. Pass the full message body you want the user to see, e.g. {\"text\": \"Here are the results: …\"}. If your previous reply was truncated, you may need to shorten it before retrying.", IsError: true, Trusted: true}, nil } + // Validate agent-authored components BEFORE anything is published: a + // rejection returns IsError with a detailed, correctable message and leaves + // no envelope on the wire, exactly like the attachment-resolution guard + // below. Only the kind knows its component format, so the kind's validator + // (resolved from the outbound binding at construction) is the authority. + // + // pt markup is model-facing only, so the blocks a human sees are the + // STRIPPED copy — and that copy, not the tagged one, is what the validator + // must judge. Stripping can rejoin a guarded token the markers had split + // ("" publishes as ""), so + // validating the tagged bytes would pass a payload whose published form a + // check exists to refuse. The tagged in.Blocks still feeds the leakage gate + // below, which needs the tags to decide, exactly as it does for in.Text. + outBlocks, _ := toolenvelope.StripPtJSON(in.Blocks) + if len(in.Blocks) > 0 { + if !t.hasComponents { + return tool.Result{Content: "respond_to_user: `blocks` (structured components) are not supported on this channel. Put your reply in `text` instead.", IsError: true, Trusted: true}, nil + } + if len(in.Attached) > 0 { + // blocks + attached in one reply is not supported yet: the + // attachment delivery path posts only Text, so the blocks would be + // silently dropped. Reject with a correctable error rather than lose + // them. Full blocks+files-in-one-reply support is deferred. + return tool.Result{Content: "respond_to_user: `blocks` and `attached` can't be combined in one reply on this channel yet. Send the structured layout in one respond_to_user call and the attachment(s) in another.", IsError: true, Trusted: true}, nil + } + if t.components == nil { + // Advertised "components" but no validator: a wiring bug, not the + // model's fault. Fail closed rather than publish unvalidated blocks. + return tool.Result{Trusted: true}, fmt.Errorf("respond_to_user: channel kind %q advertises components but supplies no validator", t.cfg.ChannelKind) + } + if err := t.components.ValidateComponents(outBlocks); err != nil { + return tool.Result{Content: err.Error(), IsError: true, Trusted: true}, nil + } + } + // Resolve attached artifact handles BEFORE publishing — any failure // returns IsError without putting the envelope on the wire so the // model can correct and retry. @@ -233,7 +293,23 @@ func (t *respondTool) Execute(ctx context.Context, args json.RawMessage, sess *t // internal config (approver subject, SpiceDB endpoint) that must not reach // channel members. if t.cfg.LeakageGate != nil { - if err := t.cfg.LeakageGate(ctx, sess, in.Text, attachments); err != nil { + // The gate measures TEXT. Components carry their own text, so feed the + // gate the block content too (projected to plain text by the kind) — + // otherwise an agent could route blockable content through `blocks` and + // bypass the gate, a fail-open egress path. Appended to the TAGGED + // in.Text the gate already runs on; if the agent copied pt-tagged + // content into a block it stays tagged in the projection and gets the + // same tag-aware treatment as tagged in.Text, otherwise it falls to the + // gate's coarse check like an attachment. pt markup is stripped from the + // published blocks (outBlocks, validated above in its stripped form), + // mirroring outText, so a human never sees the markup either way. + gateText := in.Text + if len(in.Blocks) > 0 && t.components != nil { + if projected := t.components.ComponentsPlainText(in.Blocks); projected != "" { + gateText = in.Text + "\n\n" + projected + } + } + if err := t.cfg.LeakageGate(ctx, sess, gateText, attachments); err != nil { logger := log.FromContext(ctx) pub := func(subject string, data []byte) error { return publishWithRetry(ctx, t.cfg.NATSPublish, subject, data) @@ -296,8 +372,11 @@ func (t *respondTool) Execute(ctx context.Context, args json.RawMessage, sess *t } // pt markup is model-facing only: strip any pt-untrusted envelope before the - // human sees the reply. The LeakageGate above ran on the TAGGED in.Text (it - // needs the tags to decide), so the strip is on the outbound copy only. + // human sees the reply. The LeakageGate above ran on the TAGGED in.Text and + // the TAGGED block projection (it needs the tags to decide), so the strip is + // on the outbound copies only — BOTH text and blocks (outBlocks, stripped + // and validated before the gate), so neither surface renders provenance + // markup to a human. outText := in.Text if stripped, had := toolenvelope.StripPt(in.Text); had { outText = stripped @@ -309,13 +388,13 @@ func (t *respondTool) Execute(ctx context.Context, args json.RawMessage, sess *t toolUseID = ids.ToolUseID } logger.Info("respond_to_user: publishing user_message envelope", - "session", sess.Namespace+"/"+sess.Name, "toolUseID", toolUseID, "textLen", len(outText), "attachments", len(attachments)) + "session", sess.Namespace+"/"+sess.Name, "toolUseID", toolUseID, "textLen", len(outText), "attachments", len(attachments), "components", len(in.Blocks) > 0) publish := func(subject string, data []byte) error { return publishWithRetry(ctx, t.cfg.NATSPublish, subject, data) } if err := t.cfg.EnvelopeSigner.PublishOut(publish, sess.Namespace, sess.Name, channelevents.KindUserMessage, - channelevents.OutboundUserMessagePayload{Text: outText, Attachments: attachments}, + channelevents.OutboundUserMessagePayload{Text: outText, Attachments: attachments, Components: outBlocks}, ); err != nil { return tool.Result{Content: fmt.Sprintf("publish failed: %v", err), IsError: true, Trusted: true}, nil } @@ -526,6 +605,18 @@ func ownedBySessionNamed(refs []metav1.OwnerReference, name string) bool { return false } +// normalizeOptionalBlocks returns nil for a blocks value the model meant as +// "none" — absent, whitespace, JSON null, or an empty array — so downstream +// len()>0 checks treat it as absent rather than as a present-but-empty payload +// that would error the call. A non-empty array is returned unchanged. +func normalizeOptionalBlocks(raw json.RawMessage) json.RawMessage { + switch strings.TrimSpace(string(raw)) { + case "", "null", "[]": + return nil + } + return raw +} + func publishWithRetry(ctx context.Context, publish func(ctx context.Context, subject string, payload []byte) error, subject string, payload []byte) error { var lastErr error delay := 100 * time.Millisecond diff --git a/pkg/agent/tool/meta/respond_components_test.go b/pkg/agent/tool/meta/respond_components_test.go new file mode 100644 index 00000000..cac6f08e --- /dev/null +++ b/pkg/agent/tool/meta/respond_components_test.go @@ -0,0 +1,152 @@ +package meta + +import ( + "context" + "encoding/json" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authzed/openagentprimitives/pkg/agent/tool" + "github.com/authzed/openagentprimitives/pkg/agent/toolenvelope" + "github.com/authzed/openagentprimitives/pkg/channels/channelevents" + // Registers the slack kind so ChannelKind "slack" resolves its real + // ComponentFormatter/ComponentValidator — the same wiring the runner uses. + _ "github.com/authzed/openagentprimitives/pkg/channels/channelkinds/slack" + "github.com/authzed/openagentprimitives/pkg/memory" +) + +func respondCtx() context.Context { + return memory.WithSystemApproval(context.Background(), "test") +} + +func TestRespondSchemaIncludesBlocksWithComponents(t *testing.T) { + tt := newRespondTool(RespondConfig{ + Capabilities: []string{"text", "markdown", "components"}, + ChannelKind: "slack", + }) + var schema map[string]any + require.NoError(t, json.Unmarshal(tt.InputSchema(), &schema)) + props, _ := schema["properties"].(map[string]any) + + blocks, ok := props["blocks"].(map[string]any) + require.True(t, ok, "blocks property present when 'components' capability is set") + desc, _ := blocks["description"].(string) + assert.Contains(t, desc, "rich_text", "blocks description carries the kind's authoring instructions, not the old stub text") + assert.Contains(t, tt.Description(), "blocks", "tool description advertises components") +} + +func TestRespondComponentsNotSupportedChannel_IsErrorNoPublish(t *testing.T) { + calls := 0 + tt := newRespondTool(RespondConfig{ + Capabilities: []string{"text"}, // no "components" + ChannelKind: "fake", + NATSPublish: func(context.Context, string, []byte) error { calls++; return nil }, + NATSSubjectPrefix: "ap.session.default.foo", + }) + res, err := tt.Execute(respondCtx(), + json.RawMessage(`{"text":"hi","blocks":[{"type":"divider"}]}`), + &tool.SessionContext{Namespace: "default", Name: "foo"}) + require.NoError(t, err) + assert.True(t, res.IsError, "blocks on a channel without the components capability must be an error") + assert.Contains(t, res.Content, "not supported") + assert.Equal(t, 0, calls, "nothing may be published when components are unsupported") +} + +func TestRespondComponentsValidationFailure_IsErrorNoPublish(t *testing.T) { + calls := 0 + tt := newRespondTool(RespondConfig{ + Capabilities: []string{"text", "components"}, + ChannelKind: "slack", + NATSPublish: func(context.Context, string, []byte) error { calls++; return nil }, + NATSSubjectPrefix: "ap.session.default.foo", + }) + // An actions block is rejected by the slack validator (interactive). + args := `{"text":"hi","blocks":[{"type":"actions","elements":[{"type":"button","text":{"type":"plain_text","text":"Go"},"action_id":"a"}]}]}` + res, err := tt.Execute(respondCtx(), json.RawMessage(args), + &tool.SessionContext{Namespace: "default", Name: "foo"}) + require.NoError(t, err) + assert.True(t, res.IsError, "invalid components must surface IsError") + assert.Contains(t, res.Content, "interactive", "the detailed validator error must reach the model") + assert.Equal(t, 0, calls, "no envelope may be published when validation fails") +} + +func TestRespondComponentsSuccess_CarriesComponentsOnPayload(t *testing.T) { + var published [][]byte + tt := newRespondTool(RespondConfig{ + Capabilities: []string{"text", "markdown", "components"}, + ChannelKind: "slack", + NATSPublish: func(_ context.Context, _ string, p []byte) error { published = append(published, p); return nil }, + NATSSubjectPrefix: "ap.session.default.foo", + }) + blocksJSON := `[{"type":"section","text":{"type":"mrkdwn","text":"*hi*"}}]` + args := `{"text":"summary","blocks":` + blocksJSON + `}` + res, err := tt.Execute(respondCtx(), json.RawMessage(args), + &tool.SessionContext{Namespace: "default", Name: "foo"}) + require.NoError(t, err) + require.False(t, res.IsError, "valid components must publish; got: %s", res.Content) + require.Len(t, published, 1) + + var env channelevents.Envelope + require.NoError(t, json.Unmarshal(published[0], &env)) + var pl channelevents.OutboundUserMessagePayload + require.NoError(t, json.Unmarshal(env.Payload, &pl)) + assert.Equal(t, "summary", pl.Text, "text is carried as the fallback") + assert.JSONEq(t, blocksJSON, string(pl.Components), "validated components are carried on the payload") +} + +// The leakage gate measures text; without feeding it the block content, an agent +// could route blockable content through blocks and bypass the gate. This proves +// the gate sees the block text even when the reply text is innocuous. +func TestRespondComponentsLeakageGateSeesBlockText(t *testing.T) { + var sawGateText string + tt := newRespondTool(RespondConfig{ + Capabilities: []string{"text", "components"}, + ChannelKind: "slack", + NATSPublish: func(context.Context, string, []byte) error { return nil }, + NATSSubjectPrefix: "ap.session.default.foo", + LeakageGate: func(_ context.Context, _ *tool.SessionContext, text string, _ []channelevents.AttachmentRef) error { + sawGateText = text + if strings.Contains(text, "SECRET") { + return errors.New("blocked by audience check") + } + return nil + }, + }) + // "SECRET" appears only in the block, not in the reply text. + args := `{"text":"all clear","blocks":[{"type":"section","text":{"type":"mrkdwn","text":"the SECRET value is 42"}}]}` + res, err := tt.Execute(respondCtx(), json.RawMessage(args), + &tool.SessionContext{Namespace: "default", Name: "foo"}) + require.NoError(t, err) + assert.Contains(t, sawGateText, "SECRET", "the gate must measure block text, not just the reply text") + assert.True(t, res.IsError, "the gate blocked on content carried only in blocks") +} + +// pt stripping can REJOIN a token the markers split, so the validator must +// judge the stripped blocks a human will see, not the tagged ones the model +// sent: "" publishes as "". +func TestRespondComponents_PtSplitBroadcastIsRejectedAfterStrip(t *testing.T) { + calls := 0 + tt := newRespondTool(RespondConfig{ + Capabilities: []string{"text", "components"}, + ChannelKind: "slack", + NATSPublish: func(context.Context, string, []byte) error { calls++; return nil }, + NATSSubjectPrefix: "ap.session.default.foo", + }) + split := "hey ", "n1", "pt_1") + " look" + blocks, err := json.Marshal([]map[string]any{{ + "type": "section", "text": map[string]string{"type": "mrkdwn", "text": split}, + }}) + require.NoError(t, err) + args, err := json.Marshal(map[string]any{"text": "hi", "blocks": json.RawMessage(blocks)}) + require.NoError(t, err) + + res, err := tt.Execute(respondCtx(), args, &tool.SessionContext{Namespace: "default", Name: "foo"}) + require.NoError(t, err) + assert.True(t, res.IsError, "a broadcast reassembled by the strip must be refused") + assert.Contains(t, res.Content, "broadcast") + assert.Equal(t, 0, calls, "nothing may be published") +} diff --git a/pkg/agent/toolenvelope/pt_json.go b/pkg/agent/toolenvelope/pt_json.go new file mode 100644 index 00000000..7b6dabb9 --- /dev/null +++ b/pkg/agent/toolenvelope/pt_json.go @@ -0,0 +1,62 @@ +package toolenvelope + +import ( + "bytes" + "encoding/json" +) + +// StripPtJSON removes pt-untrusted envelope markers from every string value in +// a JSON document, keeping the wrapped content, and reports whether anything +// was stripped. It is StripPt for structured payloads: tool arguments (so a +// tool never sees markup) and agent-authored components (so a human never +// does). Both consumers share this one walker so they cannot drift. +// +// The markup rides inside string VALUES (quotes escaped in the raw bytes), so +// it walks the decoded string leaves. Numbers are decoded with UseNumber so a +// large-integer sibling of a tagged field (a Snowflake id, a nanosecond +// timestamp) survives the round-trip intact. On any parse failure, or when +// nothing was stripped, it returns the ORIGINAL bytes unchanged. +func StripPtJSON(raw json.RawMessage) (json.RawMessage, bool) { + if len(raw) == 0 { + return raw, false + } + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + var v any + if err := dec.Decode(&v); err != nil { + return raw, false + } + had := false + walked := walkStripPt(v, &had) + if !had { + return raw, false // nothing tagged; leave the exact bytes (no needless re-marshal) + } + out, err := json.Marshal(walked) + if err != nil { + return raw, false + } + return json.RawMessage(out), true +} + +func walkStripPt(v any, had *bool) any { + switch t := v.(type) { + case string: + s, found := StripPt(t) + if found { + *had = true + } + return s + case []any: + for i, e := range t { + t[i] = walkStripPt(e, had) + } + return t + case map[string]any: + for k, e := range t { + t[k] = walkStripPt(e, had) + } + return t + default: + return v + } +} diff --git a/pkg/agent/runner/pttag_argstrip_test.go b/pkg/agent/toolenvelope/pt_json_test.go similarity index 51% rename from pkg/agent/runner/pttag_argstrip_test.go rename to pkg/agent/toolenvelope/pt_json_test.go index 66f5914a..eca8be8f 100644 --- a/pkg/agent/runner/pttag_argstrip_test.go +++ b/pkg/agent/toolenvelope/pt_json_test.go @@ -1,4 +1,4 @@ -package runner +package toolenvelope_test import ( "encoding/json" @@ -10,32 +10,28 @@ import ( "github.com/authzed/openagentprimitives/pkg/agent/toolenvelope" ) -func TestStripArgTags_removesEnvelopeAndPreservesBigInt(t *testing.T) { +func TestStripPtJSON_removesEnvelopeAndPreservesBigInt(t *testing.T) { inner := toolenvelope.WrapPt("secret data", "n1", "pt_1") args, err := json.Marshal(map[string]any{"text": inner, "big": json.Number("9007199254740993")}) require.NoError(t, err) - out := stripArgTags(args) + out, had := toolenvelope.StripPtJSON(args) + assert.True(t, had, "a tagged string leaf reports stripped") var got map[string]any require.NoError(t, json.Unmarshal(out, &got)) - assert.Equal(t, "secret data", got["text"], "the tool sees clean content, no pt markers") - assert.NotContains(t, string(out), "pt-untrusted", "no envelope markers reach the tool") + assert.Equal(t, "secret data", got["text"], "the consumer sees clean content, no pt markers") + assert.NotContains(t, string(out), "pt-untrusted", "no envelope markers survive") // The large-int sibling of the tagged field survives exactly (UseNumber), // even though the payload was re-marshaled because a tag was stripped. assert.Contains(t, string(out), "9007199254740993", "a large integer is not truncated when a sibling field was stripped") } -func TestStripArgTags_untaggedIsByteIdentical(t *testing.T) { - args := json.RawMessage(`{"text":"plain content","big":9007199254740993}`) - out := stripArgTags(args) - assert.Equal(t, string(args), string(out), - "untagged args are returned byte-for-byte (no re-marshal, no precision risk)") -} - -func TestStripArgTags_malformedJSONUnchanged(t *testing.T) { - args := json.RawMessage(`not json`) - assert.Equal(t, string(args), string(stripArgTags(args)), - "unparseable args are handed to the tool exactly as sent") +func TestStripPtJSON_untaggedOrMalformedIsByteIdentical(t *testing.T) { + for _, raw := range []string{`{"text":"plain content","big":9007199254740993}`, `not json`, ``} { + out, had := toolenvelope.StripPtJSON(json.RawMessage(raw)) + assert.False(t, had, "%q: nothing stripped", raw) + assert.Equal(t, raw, string(out), "%q: returned byte-for-byte (no re-marshal, no precision risk)", raw) + } } diff --git a/pkg/channels/channelevents/envelope.go b/pkg/channels/channelevents/envelope.go index 787bc319..eef81386 100644 --- a/pkg/channels/channelevents/envelope.go +++ b/pkg/channels/channelevents/envelope.go @@ -239,6 +239,24 @@ type OutboundUserMessagePayload struct { // time via the operator's HTTP /artifact/... endpoint and never travel // via NATS. Empty means a text-only reply. Attachments []AttachmentRef `json:"attachments,omitempty"` + // Components is agent-authored structured UI in the OUTBOUND kind's native + // format (Slack Block Kit today), already validated by that kind's + // ComponentValidator before this payload was published. The kind's Sender + // renders it as the message body with Text as the notification/degrade + // fallback; a kind that does not support components ignores it. Empty means + // a text-only (or text+attachments) reply. Opaque to channelsd — format is + // a kind concern, never an envelope one. + // + // Version skew: this field arrived with the runner advertising a `blocks` + // capability. A channelsd predating it unmarshals this struct without a + // Components field, silently drops the value, and posts Text alone. There is + // no runner<->channelsd capability handshake, so the two should be upgraded + // together. The degradation is graceful, not silent loss: the model is + // required to fill Text with a self-contained concise summary (the same + // render fallback used when blocks can't render), so a skewed old channelsd + // posts that summary rather than a blank reply. A runtime capability + // negotiation is deferred. + Components json.RawMessage `json:"components,omitempty"` } // AttachmentRef references an ArtifactRender CR whose status.outputRef diff --git a/pkg/channels/channelevents/envelope_components_test.go b/pkg/channels/channelevents/envelope_components_test.go new file mode 100644 index 00000000..bf78807c --- /dev/null +++ b/pkg/channels/channelevents/envelope_components_test.go @@ -0,0 +1,29 @@ +package channelevents + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOutboundUserMessagePayloadComponents(t *testing.T) { + t.Run("present when set: round-trips", func(t *testing.T) { + p := OutboundUserMessagePayload{Text: "hi", Components: json.RawMessage(`[{"type":"divider"}]`)} + b, err := json.Marshal(p) + require.NoError(t, err) + assert.Contains(t, string(b), `"components"`) + + var back OutboundUserMessagePayload + require.NoError(t, json.Unmarshal(b, &back)) + assert.Equal(t, "hi", back.Text) + assert.JSONEq(t, `[{"type":"divider"}]`, string(back.Components)) + }) + + t.Run("omitted when empty", func(t *testing.T) { + b, err := json.Marshal(OutboundUserMessagePayload{Text: "hi"}) + require.NoError(t, err) + assert.NotContains(t, string(b), "components") + }) +} diff --git a/pkg/channels/channelkinds/components.go b/pkg/channels/channelkinds/components.go new file mode 100644 index 00000000..2dc528d3 --- /dev/null +++ b/pkg/channels/channelkinds/components.go @@ -0,0 +1,71 @@ +// How a channel kind's surface renders structured UI components — the optional +// seam that lets an agent author native layout (Slack Block Kit today) through +// respond_to_user, parallel to TextFormatter for the plain-text dialect. +package channelkinds + +import "encoding/json" + +// ComponentFormatter is the OPTIONAL interface a Kind implements to declare that +// its surface can render structured UI components and to supply the model-facing +// authoring instructions for them. +// +// The returned string is MODEL-FACING PROMPT TEXT, appended to the `blocks` +// property description of respond_to_user's input schema exactly as +// TextFormatter's instructions are appended to `text`. +// +// Unlike TextFormatter — which every registered kind must implement, because +// every surface renders text — this is genuinely optional per kind: components +// are an opt-in capability, advertised by the "components" token in +// Capabilities(). There is therefore no default and no registry-wide sweep. +type ComponentFormatter interface { + ComponentFormattingInstructions() string +} + +// ComponentValidator is the OPTIONAL interface a Kind implements to validate +// agent-authored components before they are published, and to project them to +// plain text so egress gates can measure their content. +// +// respond_to_user calls ValidateComponents SYNCHRONOUSLY in the runner, so a +// rejection returns to the model as an error it can correct and retry, rather +// than failing later at render time. The returned error is model-facing: name +// the offending block and say how to fix it. A byte-identical input must always +// validate the same way — no wall-clock, no randomness. +// +// ComponentsPlainText exists so the info-leakage egress gate measures the TEXT +// carried in components, not only the reply's `text` field: without it, an +// agent could route blockable content through `blocks` and bypass the gate — a +// fail-open path. It is best-effort: on input that will not parse it returns "" +// (ValidateComponents is the authority on rejection; this is only for +// measurement). +type ComponentValidator interface { + ValidateComponents(raw json.RawMessage) error + ComponentsPlainText(raw json.RawMessage) string +} + +// ComponentFormattingInstructionsFor returns k's component authoring +// instructions, or "" when k is nil (an unregistered kind name) or does not +// implement ComponentFormatter. +// +// There is deliberately no non-empty default: a kind that advertises the +// "components" capability is expected to implement ComponentFormatter, and an +// empty string from a misconfigured one is honest, where a fabricated default +// would describe a dialect the surface does not speak. +func ComponentFormattingInstructionsFor(k Kind) string { + cf, ok := k.(ComponentFormatter) + if !ok || cf == nil { + return "" + } + return cf.ComponentFormattingInstructions() +} + +// ComponentValidatorFor returns k's ComponentValidator, or nil when k is nil or +// does not implement it. The caller treats a nil validator on a +// components-advertising channel as fail-closed (reject), not as "skip +// validation". +func ComponentValidatorFor(k Kind) ComponentValidator { + cv, ok := k.(ComponentValidator) + if !ok || cv == nil { + return nil + } + return cv +} diff --git a/pkg/channels/channelkinds/components_test.go b/pkg/channels/channelkinds/components_test.go new file mode 100644 index 00000000..5cc4289e --- /dev/null +++ b/pkg/channels/channelkinds/components_test.go @@ -0,0 +1,50 @@ +package channelkinds + +import ( + "encoding/json" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// componentKind embeds the Kind interface (nil — its methods are never called) +// and adds the optional component seam, so the lookup helpers can be exercised +// without constructing a full Kind or importing a concrete channel package. +type componentKind struct { + Kind +} + +func (componentKind) ComponentFormattingInstructions() string { return "author blocks like so" } + +func (componentKind) ValidateComponents(raw json.RawMessage) error { + if string(raw) == "bad" { + return errors.New("bad components") + } + return nil +} + +func (componentKind) ComponentsPlainText(raw json.RawMessage) string { + return "flattened:" + string(raw) +} + +// plainKind (a bare struct{ Kind }, declared in identity_helpers_test.go) stands +// in for a kind implementing neither component interface. + +func TestComponentFormattingInstructionsFor(t *testing.T) { + assert.Equal(t, "", ComponentFormattingInstructionsFor(nil), "nil kind has no instructions") + assert.Equal(t, "", ComponentFormattingInstructionsFor(plainKind{}), "non-component kind has no instructions") + assert.Equal(t, "author blocks like so", ComponentFormattingInstructionsFor(componentKind{}), "component kind returns its instructions") +} + +func TestComponentValidatorFor(t *testing.T) { + assert.Nil(t, ComponentValidatorFor(nil), "nil kind has no validator") + assert.Nil(t, ComponentValidatorFor(plainKind{}), "non-component kind has no validator") + + v := ComponentValidatorFor(componentKind{}) + require.NotNil(t, v, "component kind exposes a validator") + assert.NoError(t, v.ValidateComponents(json.RawMessage(`ok`))) + assert.Error(t, v.ValidateComponents(json.RawMessage(`bad`))) + assert.Equal(t, "flattened:ok", v.ComponentsPlainText(json.RawMessage(`ok`)), "validator projects components to text for egress measurement") +} diff --git a/pkg/channels/channelkinds/slack/channel_history_test.go b/pkg/channels/channelkinds/slack/channel_history_test.go index 9a852f35..5f714ad2 100644 --- a/pkg/channels/channelkinds/slack/channel_history_test.go +++ b/pkg/channels/channelkinds/slack/channel_history_test.go @@ -21,7 +21,7 @@ type fakeHistClient struct { func (f *fakeHistClient) GetConversationRepliesContext(context.Context, *slackapi.GetConversationRepliesParameters) ([]slackapi.Message, bool, string, error) { return nil, false, "", nil } -func (f *fakeHistClient) GetUserInfoContext(_ context.Context, id string) (*slackapi.User, error) { +func (f *fakeHistClient) GetUserInfoContext(_ context.Context, id string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { return &slackapi.User{Name: "u" + id, RealName: "User " + id, Profile: slackapi.UserProfile{Email: id + "@x.com"}}, nil } func (f *fakeHistClient) GetConversationHistoryContext(_ context.Context, p *slackapi.GetConversationHistoryParameters) (*slackapi.GetConversationHistoryResponse, error) { diff --git a/pkg/channels/channelkinds/slack/components.go b/pkg/channels/channelkinds/slack/components.go new file mode 100644 index 00000000..b3b70a66 --- /dev/null +++ b/pkg/channels/channelkinds/slack/components.go @@ -0,0 +1,534 @@ +package slack + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "strings" + "unicode/utf8" + + slackapi "github.com/slack-go/slack" +) + +// Slack's documented Block Kit limits for the presentational blocks this +// channel accepts from an agent. Enforcing them in the runner — before the +// envelope is published — turns a would-be post-time rejection (which degrades +// to plain text) into a precise, correctable error the model sees on the +// respond_to_user call. Every text limit is in characters (runes), as Slack +// counts them — a byte count would reject non-Latin text well under the limit. +// The per-message block and section caps are the sender's own constants +// (sender_chunk.go), so validation and rendering share one source of truth. +const ( + // maxAgentBlocks reserves one of Slack's 50 block slots for the + // sender-appended mention_only "@-mention me to reply" footer, which a + // mention_only thread cannot do without (a bare reply there never routes). + // The validator has no routing-mode input, so it reserves the slot always. + maxAgentBlocks = slackMaxBlocksPerMessage - 1 + maxSectionTextLen = slackSectionTextLimit + maxHeaderTextLen = 150 + maxContextElements = 10 + maxSectionFields = 10 + maxFieldTextLen = 2000 + maxMarkdownTextLen = 12000 + maxDataVizTitleLen = 50 + maxContainerChildren = 10 + maxChartsPerMessage = 2 + maxTablesPerMessage = 1 + maxTableRows = 100 + maxTableColumns = 20 +) + +// componentFormattingInstructions is the model-facing authoring guide for the +// `blocks` field, injected into respond_to_user the same way the mrkdwn dialect +// instructions are injected into `text`. It names the allowlist, the limits, +// and the prohibitions so the model stays inside what ValidateComponents +// accepts. +const componentFormattingInstructions = `This channel can render structured Block Kit layout. ` + + `To use it, pass a ` + "`blocks`" + ` array of Slack Block Kit blocks alongside your text. ` + + `Supported (read-only) blocks: ` + + `"header" (plain_text, ≤150 chars); ` + + `"section" (a mrkdwn "text" object and/or up to 10 "fields"; no accessory); ` + + `"divider"; ` + + `"context" (text/mrkdwn elements, ≤10); ` + + `"rich_text" (sections, bullet/ordered lists, quotes, preformatted/code); ` + + `"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); ` + + `"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"). ` + + `Section/context text uses the same Slack mrkdwn dialect as the text field (*bold*, _italic_, ` + "`code`" + `, ) — not CommonMark; the "markdown" block is the exception and takes standard markdown. ` + + `Limits: at most 49 blocks; section text ≤3000 characters; split longer content across multiple blocks. ` + + `Every block needs content: no empty header, section, context, markdown, or table. ` + + `Not supported: interactive elements (buttons, select menus, date pickers, inputs, action/input/context_actions blocks, data_table, carousel) — this surface is presentation-only; ` + + `images and icons (coming soon); and @channel/@here/@everyone broadcast mentions plus usergroup pings. ` + + `Always also fill the text field with a concise plain summary: it is the notification preview and the fallback shown if the blocks cannot be rendered.` + +// ComponentFormattingInstructions implements channelkinds.ComponentFormatter. +func (*Kind) ComponentFormattingInstructions() string { return componentFormattingInstructions } + +// ValidateComponents implements channelkinds.ComponentValidator. +func (*Kind) ValidateComponents(raw json.RawMessage) error { return validateComponents(raw) } + +// ComponentsPlainText implements channelkinds.ComponentValidator. It projects +// the Block Kit payload to the same readable text the inbound path derives from +// blocks (flattenBlocks), so the info-leakage egress gate measures what the +// blocks say. Best-effort: unparseable input yields "" — ValidateComponents, +// not this, is the authority on rejecting bad input. +func (*Kind) ComponentsPlainText(raw json.RawMessage) string { + trimmed := bytes.TrimSpace(raw) + if len(trimmed) == 0 || trimmed[0] != '[' { + return "" + } + var blocks slackapi.Blocks + if err := json.Unmarshal(trimmed, &blocks); err != nil { + return "" + } + return flattenBlocks(blocks.BlockSet) +} + +// validateComponents checks agent-authored Block Kit JSON against the +// presentation-only contract: an allowlist of read-only layout blocks, Slack's +// structural limits, no broadcast mentions, and — for now — no images. +// Interactive blocks and elements are rejected so the feature never mints a +// click that would have to route back into the session. The returned error is +// model-facing: it names the offending block and says how to fix it. +func validateComponents(raw json.RawMessage) error { + trimmed := bytes.TrimSpace(raw) + if len(trimmed) == 0 { + return errors.New("respond_to_user: `blocks` must be a non-empty JSON array of Block Kit blocks") + } + if trimmed[0] != '[' { + return errors.New("respond_to_user: `blocks` must be a JSON array of Block Kit blocks, e.g. [{\"type\":\"section\",\"text\":{\"type\":\"mrkdwn\",\"text\":\"…\"}}]") + } + + var blocks slackapi.Blocks + if err := json.Unmarshal(trimmed, &blocks); err != nil { + return fmt.Errorf("respond_to_user: `blocks` could not be parsed as Block Kit JSON: %v", err) + } + set := blocks.BlockSet + if len(set) == 0 { + return errors.New("respond_to_user: `blocks` must contain at least one block") + } + if len(set) > maxAgentBlocks { + return fmt.Errorf("respond_to_user: too many blocks (%d); at most %d blocks per message (Slack's %d, less one reserved for a system footer)", len(set), maxAgentBlocks, slackMaxBlocksPerMessage) + } + + for i, b := range set { + if err := validateBlock(fmt.Sprintf("block %d", i), b); err != nil { + return err + } + } + + // Per-message counts Slack enforces across the whole message, containers + // included: checking only top-level blocks would let a container smuggle a + // third chart into a post Slack then rejects whole. + var charts, tables int + countBlocks(set, &charts, &tables) + if charts > maxChartsPerMessage { + return fmt.Errorf("respond_to_user: too many data_visualization blocks (%d); Slack renders at most %d charts per message — send the rest in another reply", charts, maxChartsPerMessage) + } + if tables > maxTablesPerMessage { + return fmt.Errorf("respond_to_user: too many table blocks (%d); Slack allows %d table per message — send the rest in another reply", tables, maxTablesPerMessage) + } + return nil +} + +// countBlocks tallies chart and table blocks, descending into containers. +func countBlocks(set []slackapi.Block, charts, tables *int) { + for _, b := range set { + switch blk := b.(type) { + case *slackapi.DataVisualizationBlock: + *charts++ + case *slackapi.TableBlock: + *tables++ + case *slackapi.ContainerBlock: + countBlocks(blk.ChildBlocks.BlockSet, charts, tables) + } + } +} + +// runeLen counts characters the way Slack's limits do. +func runeLen(s string) int { return utf8.RuneCountInString(s) } + +func blockErrf(loc, typ, format string, args ...any) error { + return fmt.Errorf("respond_to_user: %s (%s): "+format, append([]any{loc, typ}, args...)...) +} + +// validateBlock validates one block (loc is a human path like "block 2" or +// "block 2 child 1"). Read-only layout blocks pass; interactive, image, and +// unknown blocks are rejected with a model-facing reason. +func validateBlock(loc string, b slackapi.Block) error { + switch blk := b.(type) { + case *slackapi.HeaderBlock: + if blk.Text == nil || strings.TrimSpace(blk.Text.Text) == "" { + return blockErrf(loc, "header", "requires non-empty \"text\" ({\"type\":\"plain_text\",\"text\":…})") + } + if n := runeLen(blk.Text.Text); n > maxHeaderTextLen { + return blockErrf(loc, "header", "text is %d characters; Slack limits header text to %d", n, maxHeaderTextLen) + } + return checkBroadcast(loc, "header", blk.Text.Text) + case *slackapi.SectionBlock: + return validateSection(loc, blk) + case *slackapi.DividerBlock: + return nil + case *slackapi.ContextBlock: + return validateContext(loc, blk) + case *slackapi.RichTextBlock: + return validateRichText(loc, blk) + case *slackapi.MarkdownBlock: + if strings.TrimSpace(blk.Text) == "" { + return blockErrf(loc, "markdown", "requires non-empty \"text\"") + } + if n := runeLen(blk.Text); n > maxMarkdownTextLen { + return blockErrf(loc, "markdown", "text is %d characters; Slack limits a markdown block to %d", n, maxMarkdownTextLen) + } + return checkBroadcast(loc, "markdown", blk.Text) + case *slackapi.TableBlock: + return validateTable(loc, blk) + case *slackapi.DataVisualizationBlock: + return validateDataViz(loc, blk) + case *slackapi.CardBlock: + return validateCard(loc, blk) + case *slackapi.ContainerBlock: + return validateContainer(loc, blk) + case *slackapi.PlanBlock: + if err := checkBroadcast(loc, "plan", blk.Title); err != nil { + return err + } + for j := range blk.Tasks { + if err := validateTaskCard(fmt.Sprintf("%s task %d", loc, j), &blk.Tasks[j]); err != nil { + return err + } + } + return nil + case *slackapi.TaskCardBlock: + return validateTaskCard(loc, blk) + case *slackapi.AlertBlock: + return blockErrf(loc, "alert", "alert blocks are only valid in modals, not in messages; use a section (with *bold* and an emoji) or a markdown block for an inline callout") + case *slackapi.ImageBlock: + return blockErrf(loc, "image", "image blocks are not yet supported on this channel; omit them for now") + case *slackapi.ActionBlock, *slackapi.InputBlock, *slackapi.ContextActionsBlock: + return blockErrf(loc, string(b.BlockType()), "interactive components (buttons, menus, inputs) are not supported; this channel accepts presentation-only layout") + case *slackapi.DataTableBlock: + return blockErrf(loc, "data_table", "the data_table block is interactive; use a read-only \"table\" block instead") + case *slackapi.CarouselBlock: + return blockErrf(loc, "carousel", "the carousel block is interactive (its cards carry actions); it is not supported on this channel") + default: + return blockErrf(loc, string(b.BlockType()), "unsupported block type; allowed: header, section, divider, context, rich_text, markdown, table, data_visualization, plan, task_card, card, container") + } +} + +func validateSection(loc string, s *slackapi.SectionBlock) error { + // A section accessory is either an image (slice 2) or an interactive element + // (never): reject any accessory rather than enumerate the ways it's unsupported. + if s.Accessory != nil { + return blockErrf(loc, "section", "accessories are not supported on this channel; use plain section text and fields") + } + hasText := s.Text != nil && strings.TrimSpace(s.Text.Text) != "" + if !hasText && len(s.Fields) == 0 { + return blockErrf(loc, "section", "requires a non-empty \"text\" object or at least one entry in \"fields\"") + } + if s.Text != nil { + if n := runeLen(s.Text.Text); n > maxSectionTextLen { + return blockErrf(loc, "section", "text is %d characters; Slack limits section text to %d — split it across multiple section blocks", n, maxSectionTextLen) + } + if err := checkBroadcast(loc, "section", s.Text.Text); err != nil { + return err + } + } + if len(s.Fields) > maxSectionFields { + return blockErrf(loc, "section", "has %d fields; Slack limits a section to %d fields", len(s.Fields), maxSectionFields) + } + for _, f := range s.Fields { + if f == nil { + continue + } + if n := runeLen(f.Text); n > maxFieldTextLen { + return blockErrf(loc, "section", "a field is %d characters; Slack limits each field to %d", n, maxFieldTextLen) + } + if err := checkBroadcast(loc, "section", f.Text); err != nil { + return err + } + } + return nil +} + +func validateContext(loc string, c *slackapi.ContextBlock) error { + els := c.ContextElements.Elements + if len(els) == 0 { + return blockErrf(loc, "context", "requires at least one text element in \"elements\"") + } + if len(els) > maxContextElements { + return blockErrf(loc, "context", "has %d elements; Slack limits a context block to %d elements", len(els), maxContextElements) + } + for _, el := range els { + text, ok := el.(*slackapi.TextBlockObject) + if !ok { + // The only non-text context element is an image, deferred to slice 2. + return blockErrf(loc, "context", "image elements are not yet supported on this channel; use text-only context") + } + if err := checkBroadcast(loc, "context", text.Text); err != nil { + return err + } + } + return nil +} + +func validateRichText(loc string, rt *slackapi.RichTextBlock) error { + if richTextElementsHaveBroadcast(rt.Elements) { + return blockErrf(loc, "rich_text", "broadcast or usergroup mentions (@channel, @here, @everyone, ) are not allowed in agent-authored components") + } + return nil +} + +func richTextElementsHaveBroadcast(els []slackapi.RichTextElement) bool { + for _, el := range els { + if richTextHasBroadcast(el) { + return true + } + } + return false +} + +func validateTable(loc string, t *slackapi.TableBlock) error { + if len(t.Rows) == 0 { + return blockErrf(loc, "table", "requires at least one row in \"rows\"") + } + if len(t.Rows) > maxTableRows { + return blockErrf(loc, "table", "has %d rows; Slack limits a table to %d rows — split it or summarize", len(t.Rows), maxTableRows) + } + for r, row := range t.Rows { + if len(row) > maxTableColumns { + return blockErrf(loc, "table", "row %d has %d cells; Slack limits a table to %d columns", r, len(row), maxTableColumns) + } + for c, cell := range row { + cloc := fmt.Sprintf("%s table r%dc%d", loc, r, c) + switch cc := cell.(type) { + case nil: + // empty cell (Slack sends null) — nothing to validate. + case *slackapi.TableRichTextCell: + if richTextElementsHaveBroadcast(cc.Elements) { + return blockErrf(cloc, "table", "broadcast or usergroup mentions (@channel, @here, @everyone, ) are not allowed in agent-authored components") + } + case *slackapi.TableRawTextCell: + if err := checkBroadcast(cloc, "table", cc.Text); err != nil { + return err + } + case *slackapi.TableRawNumberCell: + return blockErrf(cloc, "table", "raw_number cells are rejected by chat.postMessage; use a raw_text cell for a number in a message") + default: + return blockErrf(cloc, "table", "unsupported table cell type") + } + } + } + return nil +} + +// 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. +// +// 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 +// categories, and axis labels — not just the title. Those strings render in +// Slack (a or fits a 20-char label), so checking only +// the title would leave a mass-ping path open. The set mirrors flattenDataViz, +// so validation and the leakage-gate projection stay in sync. +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 := checkBroadcast(loc, "data_visualization", dv.Title); err != nil { + return err + } + 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 + } + } + case *slackapi.DataVisualizationBarChart: + return validateVizSeries(loc, "bar", c.Series, c.AxisConfig) + case *slackapi.DataVisualizationAreaChart: + return validateVizSeries(loc, "area", c.Series, c.AxisConfig) + case *slackapi.DataVisualizationLineChart: + return validateVizSeries(loc, "line", c.Series, c.AxisConfig) + default: + return blockErrf(loc, "data_visualization", "requires a \"chart\" object whose \"type\" is one of pie, bar, area, line") + } + 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) + } + for _, s := range series { + if err := checkBroadcast(loc, "data_visualization", s.Name); err != nil { + return err + } + for _, p := range s.Data { + if err := checkBroadcast(loc, "data_visualization", p.Label); err != nil { + return err + } + } + } + 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) +} + +func validateContainer(loc string, c *slackapi.ContainerBlock) error { + // Slack rejects a titleless container: "must define either `title`, or + // `rich_text_title`" (verified against the live API). + if c.Title == nil && c.RichTextTitle == nil { + return blockErrf(loc, "container", "requires a \"title\" (plain_text/mrkdwn) or a \"rich_text_title\"") + } + if c.Icon != nil { + return blockErrf(loc, "container", "container icons are not yet supported on this channel; omit them for now") + } + if c.Title != nil { + if err := checkBroadcast(loc, "container", c.Title.Text); err != nil { + return err + } + } + if c.Subtitle != nil { + if err := checkBroadcast(loc, "container", c.Subtitle.Text); err != nil { + return err + } + } + if c.RichTextTitle != nil { + if err := validateRichText(loc, c.RichTextTitle); err != nil { + return err + } + } + n := len(c.ChildBlocks.BlockSet) + if n == 0 { + return blockErrf(loc, "container", "requires at least one block in \"child_blocks\"") + } + if n > maxContainerChildren { + return blockErrf(loc, "container", "has %d child blocks; Slack limits a container to %d", n, maxContainerChildren) + } + for j, child := range c.ChildBlocks.BlockSet { + if err := validateBlock(fmt.Sprintf("%s child %d", loc, j), child); err != nil { + return err + } + } + return nil +} + +func validateCard(loc string, c *slackapi.CardBlock) error { + if c.Actions != nil { + return blockErrf(loc, "card", "card action buttons are interactive and not supported; use a card with only title/subtitle/body/subtext") + } + if c.HeroImage != nil || c.Icon != nil || c.SlackIcon != nil { + return blockErrf(loc, "card", "card images and icons are not yet supported on this channel; omit them for now") + } + for _, t := range []*slackapi.TextBlockObject{c.Title, c.Subtitle, c.Body, c.Subtext} { + if t == nil { + continue + } + if err := checkBroadcast(loc, "card", t.Text); err != nil { + return err + } + } + return nil +} + +func validateTaskCard(loc string, tc *slackapi.TaskCardBlock) error { + if err := checkBroadcast(loc, "task_card", tc.Title); err != nil { + return err + } + if tc.Details != nil { + if err := validateRichText(loc, tc.Details); err != nil { + return err + } + } + if tc.Output != nil { + if err := validateRichText(loc, tc.Output); err != nil { + return err + } + } + for _, s := range tc.Sources { + if err := checkBroadcast(loc, "task_card", s.Text); err != nil { + return err + } + } + return nil +} + +// richTextHasBroadcast walks the rich-text element tree looking for a broadcast +// element (the rich_text encoding of @channel/@here/@everyone). +func richTextHasBroadcast(el slackapi.RichTextElement) bool { + switch e := el.(type) { + case *slackapi.RichTextSection: + return sectionElemsHaveBroadcast(e.Elements) + case *slackapi.RichTextQuote: + return sectionElemsHaveBroadcast(e.Elements) + case *slackapi.RichTextPreformatted: + return sectionElemsHaveBroadcast(e.Elements) + case *slackapi.RichTextList: + for _, sub := range e.Elements { + if richTextHasBroadcast(sub) { + return true + } + } + } + return false +} + +func sectionElemsHaveBroadcast(els []slackapi.RichTextSectionElement) bool { + for _, el := range els { + switch el.(type) { + case *slackapi.RichTextSectionBroadcastElement, *slackapi.RichTextSectionUserGroupElement: + // broadcast = @channel/@here/@everyone; usergroup = , + // which notifies every member — both are mass-notification pings. + return true + } + } + return false +} + +func checkBroadcast(loc, typ, text string) error { + if containsBroadcastMention(text) { + return blockErrf(loc, typ, "broadcast or usergroup mentions (@channel, @here, @everyone, ) are not allowed in agent-authored components") + } + return nil +} + +// containsBroadcastMention reports whether text carries a Slack +// mass-notification span. Slack writes broadcasts as , , or +// , and a usergroup ping (which notifies every member) as +// , all optionally with a |label — so matching the opening token +// catches every form. +func containsBroadcastMention(text string) bool { + for _, tok := range []string{""}, + }, + { + name: "task_card source: URL projected", + json: `[{"type":"task_card","task_id":"t1","title":"Check","sources":[{"type":"url","url":"https://x.example/?k=SECRET","text":"ref"}]}]`, + want: []string{""}, + }, + { + name: "table: one line per row, cells separated", + json: `[{"type":"table","rows":[[{"type":"raw_text","text":"Name"},{"type":"raw_text","text":"Count"}],[{"type":"raw_text","text":"a"},{"type":"raw_text","text":"1"}]]}]`, + want: []string{"Name | Count\na | 1"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := k.ComponentsPlainText(json.RawMessage(tc.json)) + for _, w := range tc.want { + assert.Contains(t, got, w) + } + }) + } +} diff --git a/pkg/channels/channelkinds/slack/components_test.go b/pkg/channels/channelkinds/slack/components_test.go new file mode 100644 index 00000000..65e7ce1a --- /dev/null +++ b/pkg/channels/channelkinds/slack/components_test.go @@ -0,0 +1,210 @@ +package slack + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func repeatBlock(block string, n int) string { + parts := make([]string, n) + for i := range parts { + parts[i] = block + } + return "[" + strings.Join(parts, ",") + "]" +} + +func sectionWithText(n int) string { + return `[{"type":"section","text":{"type":"mrkdwn","text":"` + strings.Repeat("a", n) + `"}}]` +} + +func headerWithText(n int) string { + return `[{"type":"header","text":{"type":"plain_text","text":"` + strings.Repeat("a", n) + `"}}]` +} + +func headerWith(char string, n int) string { + return `[{"type":"header","text":{"type":"plain_text","text":"` + strings.Repeat(char, n) + `"}}]` +} + +func sectionWith(char string, n int) string { + return `[{"type":"section","text":{"type":"mrkdwn","text":"` + strings.Repeat(char, n) + `"}}]` +} + +const pieChart = `{"type":"data_visualization","title":"Share","chart":{"type":"pie","segments":[{"label":"A","value":1}]}}` + +func rawTextTable(rows, cols int) string { + cells := make([]string, cols) + for i := range cells { + cells[i] = `{"type":"raw_text","text":"x"}` + } + row := "[" + strings.Join(cells, ",") + "]" + rs := make([]string, rows) + for i := range rs { + rs[i] = row + } + return `{"type":"table","rows":[` + strings.Join(rs, ",") + `]}` +} + +func sectionWithFields(n int) string { + fs := make([]string, n) + for i := range fs { + fs[i] = `{"type":"mrkdwn","text":"f"}` + } + return `[{"type":"section","fields":[` + strings.Join(fs, ",") + `]}]` +} + +func contextWithTextElements(n int) string { + es := make([]string, n) + for i := range es { + es[i] = `{"type":"mrkdwn","text":"e"}` + } + return `[{"type":"context","elements":[` + strings.Join(es, ",") + `]}]` +} + +func TestValidateComponents(t *testing.T) { + cases := []struct { + name string + json string + wantErr string // "" means expect nil; otherwise err.Error() must contain this + }{ + // --- accepted presentational layouts --- + { + name: "header+section+divider+context: accepted", + json: `[{"type":"header","text":{"type":"plain_text","text":"Results"}},{"type":"section","text":{"type":"mrkdwn","text":"*All good.*"}},{"type":"divider"},{"type":"context","elements":[{"type":"mrkdwn","text":"as of 10:00"}]}]`, + wantErr: "", + }, + { + name: "section with fields: accepted", + json: `[{"type":"section","fields":[{"type":"mrkdwn","text":"*Status*\nOK"},{"type":"mrkdwn","text":"*Count*\n42"}]}]`, + wantErr: "", + }, + { + name: "rich_text section: accepted", + json: `[{"type":"rich_text","elements":[{"type":"rich_text_section","elements":[{"type":"text","text":"hello"}]}]}]`, + wantErr: "", + }, + { + name: "markdown block: accepted", + json: `[{"type":"markdown","text":"## Summary\n- one\n- two"}]`, + wantErr: "", + }, + { + name: "table with rich_text + raw_text cells: accepted", + json: `[{"type":"table","rows":[[{"type":"rich_text","elements":[{"type":"rich_text_section","elements":[{"type":"text","text":"Name"}]}]},{"type":"raw_text","text":"OK"}]]}]`, + wantErr: "", + }, + { + name: "data_visualization pie chart: accepted", + json: `[{"type":"data_visualization","title":"Share","chart":{"type":"pie","segments":[{"label":"A","value":60},{"label":"B","value":40}]}}]`, + wantErr: "", + }, + { + name: "card text-only: accepted", + json: `[{"type":"card","title":{"type":"plain_text","text":"Release 1.2"},"body":{"type":"mrkdwn","text":"Shipped *today*."}}]`, + wantErr: "", + }, + { + name: "container with title + read-only children: accepted", + json: `[{"type":"container","title":{"type":"plain_text","text":"Group"},"child_blocks":[{"type":"header","text":{"type":"plain_text","text":"Sub"}},{"type":"section","text":{"type":"mrkdwn","text":"inside"}}]}]`, + wantErr: "", + }, + { + name: "bar chart with series+axis_config: accepted", + json: `[{"type":"data_visualization","title":"Signups","chart":{"type":"bar","series":[{"name":"S","data":[{"label":"Mon","value":10}]}],"axis_config":{"categories":["Mon"]}}}]`, + wantErr: "", + }, + + // --- shape errors --- + {name: "empty array: rejected", json: `[]`, wantErr: "at least one block"}, + {name: "not an array: rejected", json: `{"type":"section"}`, wantErr: "must be a JSON array"}, + {name: "malformed json: rejected", json: `[{"type":`, wantErr: "could not be parsed"}, + + // --- interactivity rejected (presentation-only scope) --- + {name: "actions block: rejected", json: `[{"type":"actions","elements":[{"type":"button","text":{"type":"plain_text","text":"Go"},"action_id":"a"}]}]`, wantErr: "interactive"}, + {name: "input block: rejected", json: `[{"type":"input","label":{"type":"plain_text","text":"Name"},"element":{"type":"plain_text_input","action_id":"a"}}]`, wantErr: "interactive"}, + {name: "section with button accessory: rejected", json: `[{"type":"section","text":{"type":"mrkdwn","text":"hi"},"accessory":{"type":"button","text":{"type":"plain_text","text":"Go"},"action_id":"a"}}]`, wantErr: "accessor"}, + {name: "context_actions block: rejected", json: `[{"type":"context_actions","elements":[{"type":"button","text":{"type":"plain_text","text":"x"},"action_id":"a"}]}]`, wantErr: "interactive"}, + {name: "data_table block: rejected as interactive", json: `[{"type":"data_table","column_settings":[],"rows":[]}]`, wantErr: "table"}, + {name: "carousel block: rejected as interactive", json: `[{"type":"carousel","elements":[]}]`, wantErr: "carousel"}, + {name: "card with action buttons: rejected", json: `[{"type":"card","title":{"type":"plain_text","text":"x"},"actions":{"elements":[{"type":"button","text":{"type":"plain_text","text":"Go"},"action_id":"a"}]}}]`, wantErr: "action"}, + {name: "container with interactive child: rejected", json: `[{"type":"container","title":{"type":"plain_text","text":"G"},"child_blocks":[{"type":"actions","elements":[{"type":"button","text":{"type":"plain_text","text":"x"},"action_id":"a"}]}]}]`, wantErr: "interactive"}, + {name: "container without title: rejected", json: `[{"type":"container","child_blocks":[{"type":"section","text":{"type":"mrkdwn","text":"x"}}]}]`, wantErr: "title"}, + {name: "data_visualization without title: rejected", json: `[{"type":"data_visualization","chart":{"type":"pie","segments":[{"label":"A","value":1}]}}]`, wantErr: "title"}, + {name: "bar chart missing axis_config: rejected", json: `[{"type":"data_visualization","title":"x","chart":{"type":"bar","series":[{"name":"S","data":[{"label":"Mon","value":1}]}]}}]`, wantErr: "axis_config"}, + {name: "pie chart with empty segments: rejected", json: `[{"type":"data_visualization","title":"x","chart":{"type":"pie","segments":[]}}]`, wantErr: "segments"}, + + // --- alert is modals-only, not valid in a message --- + {name: "alert block: rejected (modals-only)", json: `[{"type":"alert","level":"warning","text":{"type":"mrkdwn","text":"heads up"}}]`, wantErr: "modals"}, + + // --- images deferred to slice 2 --- + {name: "image block: rejected as not-yet-supported", json: `[{"type":"image","image_url":"https://example.com/x.png","alt_text":"x"}]`, wantErr: "image"}, + {name: "context image element: rejected as not-yet-supported", json: `[{"type":"context","elements":[{"type":"image","image_url":"https://example.com/x.png","alt_text":"x"}]}]`, wantErr: "image"}, + {name: "card with hero image: rejected as not-yet-supported", json: `[{"type":"card","title":{"type":"plain_text","text":"x"},"hero_image":{"type":"image","image_url":"https://example.com/x.png","alt_text":"x"}}]`, wantErr: "image"}, + {name: "table raw_number cell: rejected for messages", json: `[{"type":"table","rows":[[{"type":"raw_number","value":42}]]}]`, wantErr: "raw_number"}, + + // --- unsupported block types --- + {name: "unknown block type: rejected", json: `[{"type":"made_up_block"}]`, wantErr: "unsupported block type"}, + {name: "video block: rejected as unsupported", json: `[{"type":"video","title":{"type":"plain_text","text":"x"},"video_url":"https://x","thumbnail_url":"https://x","alt_text":"x"}]`, wantErr: "unsupported block type"}, + + // --- structural limits --- + {name: "51 blocks: rejected over limit", json: repeatBlock(`{"type":"divider"}`, 51), wantErr: "too many blocks"}, + {name: "section text over 3000: rejected", json: sectionWithText(3001), wantErr: "3000"}, + {name: "header over 150: rejected", json: headerWithText(151), wantErr: "150"}, + {name: "11 section fields: rejected over limit", json: sectionWithFields(11), wantErr: "fields"}, + {name: "11 context elements: rejected over limit", json: contextWithTextElements(11), wantErr: "context"}, + + // --- broadcast mentions rejected --- + {name: "section : rejected broadcast", json: `[{"type":"section","text":{"type":"mrkdwn","text":"hey look"}}]`, wantErr: "broadcast"}, + {name: "context : rejected broadcast", json: `[{"type":"context","elements":[{"type":"mrkdwn","text":" ping"}]}]`, wantErr: "broadcast"}, + {name: "section field : rejected broadcast", json: `[{"type":"section","fields":[{"type":"mrkdwn","text":""}]}]`, wantErr: "broadcast"}, + {name: "rich_text broadcast element: rejected", json: `[{"type":"rich_text","elements":[{"type":"rich_text_section","elements":[{"type":"broadcast","range":"channel"}]}]}]`, wantErr: "broadcast"}, + {name: "markdown : rejected broadcast", json: `[{"type":"markdown","text":"ping now"}]`, wantErr: "broadcast"}, + {name: "table raw_text cell : rejected broadcast", json: `[{"type":"table","rows":[[{"type":"raw_text","text":" look"}]]}]`, wantErr: "broadcast"}, + {name: "section : rejected usergroup ping", json: `[{"type":"section","text":{"type":"mrkdwn","text":"hey look"}}]`, wantErr: "usergroup"}, + {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"}, + + // --- 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: ""}, + {name: "50 blocks: rejected (one slot reserved for the footer)", json: repeatBlock(`{"type":"divider"}`, 50), wantErr: "too many blocks"}, + + // --- limits count characters, not bytes --- + {name: "header of 150 CJK chars (450 bytes): accepted", json: headerWith("日", 150), wantErr: ""}, + {name: "header of 151 CJK chars: rejected, counted in characters", json: headerWith("日", 151), wantErr: "text is 151 characters"}, + {name: "section of 3000 CJK chars (9000 bytes): accepted", json: sectionWith("日", 3000), wantErr: ""}, + {name: "markdown of 12000 CJK chars: accepted", json: `[{"type":"markdown","text":"` + strings.Repeat("日", 12000) + `"}]`, wantErr: ""}, + + // --- per-message chart and table counts, containers included --- + {name: "2 charts: accepted", json: repeatBlock(pieChart, 2), wantErr: ""}, + {name: "3 charts: rejected", json: repeatBlock(pieChart, 3), wantErr: "too many data_visualization"}, + {name: "third chart inside a container: rejected", json: `[` + pieChart + `,` + pieChart + `,{"type":"container","title":{"type":"plain_text","text":"G"},"child_blocks":[` + pieChart + `]}]`, wantErr: "too many data_visualization"}, + {name: "2 tables: rejected", json: repeatBlock(rawTextTable(1, 1), 2), wantErr: "too many table"}, + {name: "table of 100 rows x 20 columns: accepted", json: `[` + rawTextTable(100, 20) + `]`, wantErr: ""}, + {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"}, + + // --- 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"}, + {name: "section with neither text nor fields: rejected", json: `[{"type":"section"}]`, wantErr: "fields"}, + {name: "context with no elements: rejected", json: `[{"type":"context","elements":[]}]`, wantErr: "at least one"}, + {name: "markdown with empty text: rejected", json: `[{"type":"markdown","text":""}]`, wantErr: "non-empty"}, + {name: "table with no rows: rejected", json: `[{"type":"table","rows":[]}]`, wantErr: "at least one row"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateComponents([]byte(tc.json)) + if tc.wantErr == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + }) + } +} diff --git a/pkg/channels/channelkinds/slack/fakeslack/client.go b/pkg/channels/channelkinds/slack/fakeslack/client.go index 323a8a8f..7950bf52 100644 --- a/pkg/channels/channelkinds/slack/fakeslack/client.go +++ b/pkg/channels/channelkinds/slack/fakeslack/client.go @@ -173,7 +173,7 @@ func (c *Client) SeedUser(u *slackapi.User) { c.usersEmail[u.Profile.Email] = u } } -func (c *Client) GetUserInfoContext(_ context.Context, id string) (*slackapi.User, error) { +func (c *Client) GetUserInfoContext(_ context.Context, id string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { c.mu.Lock() defer c.mu.Unlock() if u, ok := c.users[id]; ok { diff --git a/pkg/channels/channelkinds/slack/history.go b/pkg/channels/channelkinds/slack/history.go index 718724a3..e8880679 100644 --- a/pkg/channels/channelkinds/slack/history.go +++ b/pkg/channels/channelkinds/slack/history.go @@ -18,7 +18,7 @@ import ( // that (outbound-message) surface. type historyClient interface { GetConversationRepliesContext(ctx context.Context, params *slackapi.GetConversationRepliesParameters) ([]slackapi.Message, bool, string, error) - GetUserInfoContext(ctx context.Context, user string) (*slackapi.User, error) + GetUserInfoContext(ctx context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) GetConversationHistoryContext(ctx context.Context, params *slackapi.GetConversationHistoryParameters) (*slackapi.GetConversationHistoryResponse, error) GetBotInfoContext(ctx context.Context, params slackapi.GetBotInfoParameters) (*slackapi.Bot, error) } diff --git a/pkg/channels/channelkinds/slack/history_email_trust_test.go b/pkg/channels/channelkinds/slack/history_email_trust_test.go index 241fa2ce..2d5d432f 100644 --- a/pkg/channels/channelkinds/slack/history_email_trust_test.go +++ b/pkg/channels/channelkinds/slack/history_email_trust_test.go @@ -12,7 +12,7 @@ import ( // trustFakeClient answers users.info from a fixed table. type trustFakeClient struct{ users map[string]*slackapi.User } -func (c trustFakeClient) GetUserInfoContext(_ context.Context, id string) (*slackapi.User, error) { +func (c trustFakeClient) GetUserInfoContext(_ context.Context, id string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { return c.users[id], nil } diff --git a/pkg/channels/channelkinds/slack/history_scopes_test.go b/pkg/channels/channelkinds/slack/history_scopes_test.go index 7c28354f..2e33b961 100644 --- a/pkg/channels/channelkinds/slack/history_scopes_test.go +++ b/pkg/channels/channelkinds/slack/history_scopes_test.go @@ -102,7 +102,7 @@ func (c *recordingHistoryClient) AuthTestContext(_ context.Context) (*slackapi.A return &slackapi.AuthTestResponse{TeamID: histTestTeam}, nil } -func (c *recordingHistoryClient) GetUserInfoContext(_ context.Context, id string) (*slackapi.User, error) { +func (c *recordingHistoryClient) GetUserInfoContext(_ context.Context, id string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { c.calls = append(c.calls, "users.info") u := &slackapi.User{ID: id, RealName: "Member One", TeamID: histTestTeam} u.Profile.Email = "member.one@example.test" diff --git a/pkg/channels/channelkinds/slack/history_test.go b/pkg/channels/channelkinds/slack/history_test.go index 34dd5904..c5e7e059 100644 --- a/pkg/channels/channelkinds/slack/history_test.go +++ b/pkg/channels/channelkinds/slack/history_test.go @@ -43,7 +43,7 @@ func (f *fakeHistoryClient) GetConversationRepliesContext(_ context.Context, p * return f.replies, f.hasMore, f.cursor, f.repErr } -func (f *fakeHistoryClient) GetUserInfoContext(_ context.Context, user string) (*slackapi.User, error) { +func (f *fakeHistoryClient) GetUserInfoContext(_ context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { if f.userErr != nil { return nil, f.userErr } diff --git a/pkg/channels/channelkinds/slack/kind.go b/pkg/channels/channelkinds/slack/kind.go index 4f929e6d..290a647d 100644 --- a/pkg/channels/channelkinds/slack/kind.go +++ b/pkg/channels/channelkinds/slack/kind.go @@ -266,6 +266,7 @@ func (*Kind) Capabilities() []string { "text", "markdown", "plan", + "components", "asset:text/html", "asset:image/png", "asset:image/jpeg", diff --git a/pkg/channels/channelkinds/slack/listener.go b/pkg/channels/channelkinds/slack/listener.go index aeaac7a4..1ced0360 100644 --- a/pkg/channels/channelkinds/slack/listener.go +++ b/pkg/channels/channelkinds/slack/listener.go @@ -2035,7 +2035,7 @@ func (a *concreteClientAdapter) GetUserByEmailContext(ctx context.Context, email return a.c.GetUserByEmailContext(ctx, email) } -func (a *concreteClientAdapter) GetUserInfoContext(ctx context.Context, user string) (*slackapi.User, error) { +func (a *concreteClientAdapter) GetUserInfoContext(ctx context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { return a.c.GetUserInfoContext(ctx, user) } diff --git a/pkg/channels/channelkinds/slack/listener_client.go b/pkg/channels/channelkinds/slack/listener_client.go index 7299acf4..2d979edf 100644 --- a/pkg/channels/channelkinds/slack/listener_client.go +++ b/pkg/channels/channelkinds/slack/listener_client.go @@ -32,7 +32,7 @@ type listenerAPIClient interface { PostEphemeralContext(ctx context.Context, channelID, userID string, options ...slackapi.MsgOption) (string, error) // GetUserInfoContext resolves a Slack user's profile (email, team, name) // for identity resolution and the App Home tab. - GetUserInfoContext(ctx context.Context, user string) (*slackapi.User, error) + GetUserInfoContext(ctx context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) // OpenViewContext opens a modal (Show Details, Show settings, etc.). OpenViewContext(ctx context.Context, triggerID string, view slackapi.ModalViewRequest) (*slackapi.ViewResponse, error) // OpenView is the non-context views.open, used by the "Restart from diff --git a/pkg/channels/channelkinds/slack/mentiontext.go b/pkg/channels/channelkinds/slack/mentiontext.go index eb76e6cd..93ab6f47 100644 --- a/pkg/channels/channelkinds/slack/mentiontext.go +++ b/pkg/channels/channelkinds/slack/mentiontext.go @@ -44,7 +44,7 @@ const mentionLookupBudget = 24 // userNamer is the users.info half of slackClient, named separately so the // helpers below state the ONE capability they need. type userNamer interface { - GetUserInfoContext(ctx context.Context, user string) (*slackapi.User, error) + GetUserInfoContext(ctx context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) } // renderSlackEntities rewrites Slack's wire encoding for mentions, channel diff --git a/pkg/channels/channelkinds/slack/messagecontent.go b/pkg/channels/channelkinds/slack/messagecontent.go index d722d0a3..090ff6c2 100644 --- a/pkg/channels/channelkinds/slack/messagecontent.go +++ b/pkg/channels/channelkinds/slack/messagecontent.go @@ -138,6 +138,126 @@ func flattenBlocks(blocks []slackapi.Block) string { } case *slackapi.RichTextBlock: add(flattenRichText(blk.Elements)) + case *slackapi.MarkdownBlock: + add(blk.Text) + case *slackapi.TableBlock: + // One line per row, cells " | "-separated, so a table read back + // (or degraded to plain text) keeps its row structure. + for _, row := range blk.Rows { + var cells []string + for _, cell := range row { + if c := strings.TrimSpace(flattenTableCell(cell)); c != "" { + cells = append(cells, c) + } + } + add(strings.Join(cells, " | ")) + } + case *slackapi.DataVisualizationBlock: + add(flattenDataViz(blk)) + case *slackapi.CardBlock: + for _, t := range []*slackapi.TextBlockObject{blk.Title, blk.Subtitle, blk.Body, blk.Subtext} { + if t != nil { + add(t.Text) + } + } + case *slackapi.ContainerBlock: + if blk.Title != nil { + add(blk.Title.Text) + } + if blk.Subtitle != nil { + add(blk.Subtitle.Text) + } + if blk.RichTextTitle != nil { + add(flattenRichText(blk.RichTextTitle.Elements)) + } + add(flattenBlocks(blk.ChildBlocks.BlockSet)) + case *slackapi.PlanBlock: + add(blk.Title) + for i := range blk.Tasks { + add(flattenTaskCard(&blk.Tasks[i])) + } + case *slackapi.TaskCardBlock: + add(flattenTaskCard(blk)) + } + } + return strings.Join(parts, "\n") +} + +// flattenDataViz projects a chart's free-form agent-authored strings — the +// title plus every segment/data-point label, series name, and axis category/ +// label. Projecting only the title (as this case once did) let an agent, or an +// injected prompt, route blockable content into chart labels and slip it past +// the egress leakage gate, which measures ComponentsPlainText -> flattenBlocks. +// Numeric values carry no text and are skipped. +func flattenDataViz(dv *slackapi.DataVisualizationBlock) string { + var parts []string + add := func(s string) { + if s = strings.TrimSpace(s); s != "" { + parts = append(parts, s) + } + } + addSeries := func(series []slackapi.DataVisualizationDataSeries, axis slackapi.DataVisualizationAxisConfig) { + for _, s := range series { + add(s.Name) + for _, p := range s.Data { + add(p.Label) + } + } + for _, cat := range axis.Categories { + add(cat) + } + add(axis.XLabel) + add(axis.YLabel) + } + add(dv.Title) + switch c := dv.Chart.(type) { + case *slackapi.DataVisualizationPieChart: + for _, seg := range c.Segments { + add(seg.Label) + } + case *slackapi.DataVisualizationBarChart: + addSeries(c.Series, c.AxisConfig) + case *slackapi.DataVisualizationAreaChart: + addSeries(c.Series, c.AxisConfig) + case *slackapi.DataVisualizationLineChart: + addSeries(c.Series, c.AxisConfig) + } + return strings.Join(parts, "\n") +} + +// flattenTableCell renders a table cell's readable text. raw_number cells carry +// no text unless a display override is set; rich_text and raw_text cells carry +// their content. +func flattenTableCell(cell slackapi.TableCell) string { + switch c := cell.(type) { + case *slackapi.TableRichTextCell: + return flattenRichText(c.Elements) + case *slackapi.TableRawTextCell: + return c.Text + case *slackapi.TableRawNumberCell: + return c.Text + } + return "" +} + +// flattenTaskCard renders a task card's readable text (title, details, output, +// and source labels) for transcript/egress projection. +func flattenTaskCard(tc *slackapi.TaskCardBlock) string { + var parts []string + if tc.Title != "" { + parts = append(parts, tc.Title) + } + if tc.Details != nil { + parts = append(parts, flattenRichText(tc.Details.Elements)) + } + if tc.Output != nil { + parts = append(parts, flattenRichText(tc.Output.Elements)) + } + // A source's URL renders as a clickable link, so it is content the egress + // gate must measure, not just its label. + for _, s := range tc.Sources { + if src := linkText(s.URL, s.Text); src != "" { + parts = append(parts, src) } } return strings.Join(parts, "\n") @@ -164,6 +284,21 @@ func flattenRichText(elements []slackapi.RichTextElement) string { return strings.TrimRight(b.String(), "\n") } +// linkText renders a link in Slack's own mrkdwn form, — the same +// text a section carrying that link holds. The URL is always kept: it is what +// the reader clicks, so projecting only the label would let a URL (and anything +// smuggled into its query string) slip past the egress gate unmeasured. +func linkText(url, label string) string { + switch { + case url == "": + return label + case label == "" || label == url: + return "<" + url + ">" + default: + return "<" + url + "|" + label + ">" + } +} + // flattenRichTextSection concatenates the leaf elements of one rich-text // section. User/channel mentions keep their "<@Uxxx>" form so the transcript // matches how the same text appears everywhere else in the pipeline. @@ -174,11 +309,7 @@ func flattenRichTextSection(elements []slackapi.RichTextSectionElement) string { case *slackapi.RichTextSectionTextElement: b.WriteString(e.Text) case *slackapi.RichTextSectionLinkElement: - if e.Text != "" { - b.WriteString(e.Text) - } else { - b.WriteString(e.URL) - } + b.WriteString(linkText(e.URL, e.Text)) case *slackapi.RichTextSectionUserElement: b.WriteString("<@" + e.UserID + ">") case *slackapi.RichTextSectionChannelElement: diff --git a/pkg/channels/channelkinds/slack/relsync_kind.go b/pkg/channels/channelkinds/slack/relsync_kind.go index 2248abdb..c153b1ab 100644 --- a/pkg/channels/channelkinds/slack/relsync_kind.go +++ b/pkg/channels/channelkinds/slack/relsync_kind.go @@ -115,7 +115,7 @@ type slackDirectoryClient interface { GetConversationsContext(ctx context.Context, params *slackapi.GetConversationsParameters) (channels []slackapi.Channel, nextCursor string, err error) GetConversationInfoContext(ctx context.Context, input *slackapi.GetConversationInfoInput) (*slackapi.Channel, error) GetUsersInConversationContext(ctx context.Context, params *slackapi.GetUsersInConversationParameters) ([]string, string, error) - GetUserInfoContext(ctx context.Context, user string) (*slackapi.User, error) + GetUserInfoContext(ctx context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) AuthTestContext(ctx context.Context) (*slackapi.AuthTestResponse, error) } diff --git a/pkg/channels/channelkinds/slack/sender.go b/pkg/channels/channelkinds/slack/sender.go index 079504c9..0d2bc87c 100644 --- a/pkg/channels/channelkinds/slack/sender.go +++ b/pkg/channels/channelkinds/slack/sender.go @@ -59,7 +59,7 @@ type slackClient interface { // GetUserInfoContext resolves a user id to a profile. The sender needs the // direction the other two lookups do not: quoted message text carries ids, // not emails, and an id is what a reader cannot read (see userNamer). - GetUserInfoContext(ctx context.Context, user string) (*slackapi.User, error) + GetUserInfoContext(ctx context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) // OpenViewContext opens a Slack modal via views.open. Used by the // approval-flow's Show Details button to surface the full args / // MCP description / op-session refs out-of-thread. @@ -175,7 +175,28 @@ func maybeAppendMentionFooter(text, routingMode string) string { if routingMode != "mention_only" { return text } - return text + "\n\n_@-mention me to reply._" + return text + "\n\n" + mentionFooterText +} + +// mentionFooterText is the mention_only hint, appended to text replies by +// maybeAppendMentionFooter and to component bodies by componentBodyBlocks. +const mentionFooterText = "_@-mention me to reply._" + +// componentBodyBlocks is the rendered body when agent-authored components ARE +// the message: the components, plus — in a mention_only thread — the @-mention +// hint as a context block. Text (which carries the hint for text replies) is only +// the notification fallback once components are the body, so without this the +// hint never renders in-thread and a user's bare reply never routes. Shared by +// every path that renders components (chat.postMessage and the streaming +// bubble's chat.update), so no path can drop it. The validator caps agent +// blocks one below Slack's limit precisely so this slot is always free. +func componentBodyBlocks(agent []slackapi.Block, routingMode string) []slackapi.Block { + if routingMode != "mention_only" { + return agent + } + return append(agent, slackapi.NewContextBlock("", + slackapi.NewTextBlockObject("mrkdwn", mentionFooterText, false, false), + )) } func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, env channelevents.Envelope) (channelkinds.SubChannelSendResult, error) { @@ -239,6 +260,10 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e // LLMs occasionally emit HTML-escaped sequences (e.g. "&") that // Slack would render literally; unescape so users see real "&". pl.Text = slackifyText(pl.Text) + // The reply text before the mention footer: the degraded plain-text + // retries rebuild the footer AFTER the component projection, so the hint + // stays at the end of the message instead of buried mid-body. + replyText := pl.Text // Adopted (mention_only) thread: a bare reply does not route to the // agent, so append an @-mention hint. No-op for default-routing @@ -292,8 +317,35 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e ts = "" } if ts != "" { - _, _, _, err := s.client.UpdateMessageContext(ctx, channelID, ts, - slackapi.MsgOptionText(responseReadyEmoji+" "+pl.Text, false)) + // Render agent-authored components into the streaming bubble too. + // Without this, the chat.update path (the normal streamed-thread case) + // would update the bubble with the text summary alone and silently drop + // the agent's table/chart/card — text is only the notification/degrade + // fallback once components are the body. + updateOpts := []slackapi.MsgOption{ + slackapi.MsgOptionText(responseReadyEmoji+" "+pl.Text, false), + } + var componentBlocks []slackapi.Block + if len(pl.Components) > 0 { + if componentBlocks = parseComponentBlocks(pl.Components); len(componentBlocks) > 0 { + componentBlocks = componentBodyBlocks(componentBlocks, sess.Channel.RoutingMode) + updateOpts = append(updateOpts, slackapi.MsgOptionBlocks(componentBlocks...)) + } else { + logger.Info("slack: user_message components did not parse into blocks; updating streaming bubble with text only", + "session", sess.Name) + } + } + _, _, _, err := s.client.UpdateMessageContext(ctx, channelID, ts, updateOpts...) + if err != nil && len(componentBlocks) > 0 && isUnsupportedBlocksErr(err) { + // Slack rejected the block rendering (same failure the + // chat.postMessage degrade path below handles). Don't drop the + // 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()) + _, _, _, err = s.client.UpdateMessageContext(ctx, channelID, ts, + slackapi.MsgOptionText(responseReadyEmoji+" "+degradedPlainText(replyText, pl.Components, sess.Channel.RoutingMode), false)) + } if err == nil { // chat.update doesn't auto-clear assistant.threads.setStatus the // way chat.postMessage does. Explicitly clear so the "is @@ -347,7 +399,7 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e // thereafter via the App Home "Agent settings" button. sessRef := sess.Namespace + "/" + sess.Name includeSettings := s.settingsSeen == nil || !s.settingsSeen.saw(sessRef) - blocks := s.buildUserMessageBlocks(ctx, sess, pl.Text, sessRef, includeSettings, logger) + blocks, settingsAppended := s.buildUserMessageBlocks(ctx, sess, pl.Text, pl.Components, sessRef, includeSettings, logger) opts := []slackapi.MsgOption{ slackapi.MsgOptionText(pl.Text, false), // plain-text fallback for push notifications slackapi.MsgOptionBlocks(blocks...), @@ -369,7 +421,7 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e logger.Info("slack: user_message blocks rejected; retrying as plain text (degraded, no Show-settings button)", "session", sess.Name, "channelID", channelID, "err", err.Error()) usedBlocks = false - plainOpts := []slackapi.MsgOption{slackapi.MsgOptionText(pl.Text, false)} + plainOpts := []slackapi.MsgOption{slackapi.MsgOptionText(degradedPlainText(replyText, pl.Components, sess.Channel.RoutingMode), false)} if threadTS != "" { plainOpts = append(plainOpts, slackapi.MsgOptionTS(threadTS)) } @@ -378,10 +430,11 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e if err != nil { return channelkinds.SubChannelSendResult{}, fmt.Errorf("slack postMessage: %w", err) } - // Mark the once-guard only after a successful block post: the - // plain-text degraded retry above never carried the button, and a - // failed post must not consume the session's one shot at it. - if includeSettings && usedBlocks && s.settingsSeen != nil { + // Mark the once-guard only when the button was actually appended AND the + // block post succeeded: the plain-text degraded retry never carried the + // button, a failed post never delivered it, and a 50-block body can leave + // no headroom for it — none of those may consume the session's one shot. + if settingsAppended && usedBlocks && s.settingsSeen != nil { s.settingsSeen.mark(sessRef) } logger.Info("slack: user_message via chat.postMessage", @@ -430,24 +483,41 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e func (s *slackSender) buildUserMessageBlocks( ctx context.Context, sess channelkinds.SessionInfo, - text, sessRef string, + text string, + components json.RawMessage, + sessRef string, includeSettings bool, logger interface{ Info(string, ...any) }, -) []slackapi.Block { - // A section block's mrkdwn is capped at 3000 chars; a single unbounded - // block makes chat.postMessage reject the whole reply with invalid_blocks. - // Split the reply across as many section blocks as it needs, each under the - // limit, preserving all content. +) ([]slackapi.Block, bool) { + // Base body: agent-authored components when present (validated by the slack + // kind's ComponentValidator in the runner before publish), otherwise the + // reply text split into section blocks. When components ARE the body, text + // is only the notification/degrade fallback — not a duplicated section. If + // components somehow fail to parse here (they were validated upstream), fall + // back to text sections rather than drop the reply. + // + // When components are the body, componentBodyBlocks also appends the + // mention_only hint; the text-section path (including the parse-failure + // fallback) already carries it in its rendered text, so it is not doubled. + routingMode := "" + if sess.Channel != nil { + routingMode = sess.Channel.RoutingMode + } var blocks []slackapi.Block - for _, chunk := range chunkForSlackSection(text, slackSectionMaxRunes) { - blocks = append(blocks, slackapi.NewSectionBlock( - slackapi.NewTextBlockObject("mrkdwn", chunk, false, false), - nil, nil, - )) + if len(components) == 0 { + blocks = append(blocks, textSectionBlocks(text)...) + } else if agent := parseComponentBlocks(components); len(agent) > 0 { + blocks = append(blocks, componentBodyBlocks(agent, routingMode)...) + } else { + logger.Info("slack: user_message components did not parse into blocks; falling back to text sections", + "session", sess.Name) + blocks = append(blocks, textSectionBlocks(text)...) } // Muted clamp-warning context line — best-effort; never blocks the send. - if s.deps.K8sClient != nil { + // Guarded by block headroom so a full (≤50-block) agent-authored body is + // not pushed over Slack's per-message cap by a system-appended extra. + if s.deps.K8sClient != nil && len(blocks) < slackMaxBlocksPerMessage { var as spiceboxv1alpha1.AgentSession if err := s.deps.K8sClient.Get(ctx, types.NamespacedName{ Namespace: sess.Namespace, Name: sess.Name, @@ -467,14 +537,75 @@ func (s *slackSender) buildUserMessageBlocks( // Show settings button — first agent message of the session only; // settings stay reachable anytime via the App Home "Agent settings" - // button. + // button. Headroom-guarded, as with the clamp line above. The returned + // bool reports whether the button was actually appended: the caller + // consumes the session's one-shot only then (and only on a successful block + // post), so a 50-block body that leaves no room does not burn the one shot + // — the button then appears on the first later reply that has room. + settingsAppended := false if includeSettings { - blocks = append(blocks, slackapi.NewActionBlock("show_settings_actions", - settingsButton(sessRef), + if len(blocks) < slackMaxBlocksPerMessage { + blocks = append(blocks, slackapi.NewActionBlock("show_settings_actions", + settingsButton(sessRef), + )) + settingsAppended = true + } else { + logger.Info("slack: no block headroom for the first-message Show settings button; leaving the session one-shot unconsumed so it appears on a later reply", + "session", sess.Name) + } + } + + return blocks, settingsAppended +} + +// textSectionBlocks splits text into mrkdwn section blocks, each under Slack's +// 3000-char section cap so chat.postMessage never rejects the reply with +// invalid_blocks. It is the body when no components are present, and the +// fallback when components fail to parse. +func textSectionBlocks(text string) []slackapi.Block { + var out []slackapi.Block + for _, chunk := range chunkForSlackSection(text, slackSectionMaxRunes) { + out = append(out, slackapi.NewSectionBlock( + slackapi.NewTextBlockObject("mrkdwn", chunk, false, false), + nil, nil, )) } + return out +} - return blocks +// degradedPlainText builds the plain-text body used when Slack rejects the Block +// Kit rendering. When the agent authored components as the body, pl.Text is only +// a concise summary, so posting it alone would drop most of the reply — append +// the components' readable projection (the same flattenBlocks the inbound and +// egress paths use) so the content survives the degrade. Plain text is not +// subject to block validation, so this always posts. +// +// text is the reply BEFORE the mention_only footer; the footer is appended last +// so the hint closes the message rather than sitting between the summary and +// the projected body. +func degradedPlainText(text string, components json.RawMessage, routingMode string) string { + if len(components) > 0 { + if flat := flattenBlocks(parseComponentBlocks(components)); flat != "" { + if text == "" { + text = flat + } else { + text += "\n\n" + flat + } + } + } + return maybeAppendMentionFooter(text, routingMode) +} + +// parseComponentBlocks unmarshals agent-authored Block Kit JSON into slack-go +// blocks. Returns nil on any failure — the caller falls back to text so a reply +// is never dropped. The payload was already validated by the slack kind's +// ComponentValidator in the runner; this is the render-time counterpart. +func parseComponentBlocks(components json.RawMessage) []slackapi.Block { + var parsed slackapi.Blocks + if err := json.Unmarshal(components, &parsed); err != nil { + return nil + } + return parsed.BlockSet } // forgetWatchdog drops the watchdog's tracking for this session — used after diff --git a/pkg/channels/channelkinds/slack/sender_chunk.go b/pkg/channels/channelkinds/slack/sender_chunk.go index ce5c7d76..43ac66ee 100644 --- a/pkg/channels/channelkinds/slack/sender_chunk.go +++ b/pkg/channels/channelkinds/slack/sender_chunk.go @@ -11,10 +11,14 @@ package slack // dropped the agent's reply and left the user with a generic "couldn't deliver" // notice. See chunkForSlackSection and buildUserMessageBlocks. const ( + // slackSectionTextLimit is Slack's limit on a section block's text, in + // characters. The agent-components validator enforces it as-is. + slackSectionTextLimit = 3000 + // slackSectionMaxRunes caps each section block's mrkdwn payload below - // Slack's 3000-char limit, leaving headroom (mrkdwn escaping can expand a - // string, e.g. `<`→`<`). - slackSectionMaxRunes = 2900 + // slackSectionTextLimit when the sender chunks its OWN text, leaving + // headroom (mrkdwn escaping can expand a string, e.g. `<`→`<`). + slackSectionMaxRunes = slackSectionTextLimit - 100 // slackMaxBlocksPerMessage is Slack's hard limit on blocks per message. slackMaxBlocksPerMessage = 50 diff --git a/pkg/channels/channelkinds/slack/sender_chunk_test.go b/pkg/channels/channelkinds/slack/sender_chunk_test.go index 740d11e6..e0d961f2 100644 --- a/pkg/channels/channelkinds/slack/sender_chunk_test.go +++ b/pkg/channels/channelkinds/slack/sender_chunk_test.go @@ -111,7 +111,7 @@ func TestBuildUserMessageBlocks_LongReply_SplitsIntoSectionBlocksUnderLimit(t *t long := strings.Repeat("Option: lead with trust, close with the RAG demo link. ", 120) require.Greater(t, len([]rune(long)), 3000, "fixture must exceed the section limit") - blocks := s.buildUserMessageBlocks(context.Background(), sess, long, "default/foo", true, noopInfoLogger{}) + blocks, _ := s.buildUserMessageBlocks(context.Background(), sess, long, nil, "default/foo", true, noopInfoLogger{}) var sections []*slackapi.SectionBlock var actions int diff --git a/pkg/channels/channelkinds/slack/sender_components_test.go b/pkg/channels/channelkinds/slack/sender_components_test.go new file mode 100644 index 00000000..7e2f9034 --- /dev/null +++ b/pkg/channels/channelkinds/slack/sender_components_test.go @@ -0,0 +1,144 @@ +package slack + +import ( + "context" + "encoding/json" + "testing" + + slackapi "github.com/slack-go/slack" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/channels/channelevents" + "github.com/authzed/openagentprimitives/pkg/channels/channelkinds" +) + +func userMessageEnvelopeWithComponents(t *testing.T, text string, components json.RawMessage) channelevents.Envelope { + t.Helper() + pl, err := json.Marshal(channelevents.OutboundUserMessagePayload{Text: text, Components: components}) + require.NoError(t, err, "marshal payload") + return channelevents.Envelope{ + Version: 1, + Kind: channelevents.KindUserMessage, + Session: channelevents.SessionRef{Namespace: "default", Name: "foo"}, + Payload: pl, + } +} + +// Agent-authored components become the message body; the text field is the +// notification fallback, not a duplicated body section. +func TestBuildUserMessageBlocks_ComponentsAreTheBody(t *testing.T) { + s := newSender(&fakeSlackClient{}) + sess := channelkinds.SessionInfo{Namespace: "default", Name: "foo"} + components := json.RawMessage(`[{"type":"header","text":{"type":"plain_text","text":"Results"}},{"type":"section","text":{"type":"mrkdwn","text":"*all good*"}}]`) + + blocks, _ := s.buildUserMessageBlocks(context.Background(), sess, "plain summary", components, "default/foo", false, noopInfoLogger{}) + + var headers, sections int + for _, b := range blocks { + switch b.(type) { + case *slackapi.HeaderBlock: + headers++ + case *slackapi.SectionBlock: + sections++ + } + } + assert.Equal(t, 1, headers, "the agent's header block must render") + assert.GreaterOrEqual(t, sections, 1, "the agent's section block must render") + assert.NotContains(t, concatBlockText(blocks), "plain summary", + "the text field is the notification fallback, not a body section, when components are present") + assert.Contains(t, concatBlockText(blocks), "Results", "the agent's header content must be present") +} + +// Defense in depth: if components somehow fail to parse at render time (they +// were validated in the runner), the content is never dropped — the sender +// falls back to the text field as a section. +func TestBuildUserMessageBlocks_UnparseableComponentsFallBackToText(t *testing.T) { + s := newSender(&fakeSlackClient{}) + sess := channelkinds.SessionInfo{Namespace: "default", Name: "foo"} + + blocks, _ := s.buildUserMessageBlocks(context.Background(), sess, "the summary text", json.RawMessage(`not valid json`), "default/foo", false, noopInfoLogger{}) + assert.Contains(t, concatBlockText(blocks), "the summary text", + "unparseable components must fall back to the text section so nothing is lost") +} + +// Absent components, behavior is unchanged: the text renders as section blocks. +func TestBuildUserMessageBlocks_NoComponentsUsesText(t *testing.T) { + s := newSender(&fakeSlackClient{}) + sess := channelkinds.SessionInfo{Namespace: "default", Name: "foo"} + + blocks, _ := s.buildUserMessageBlocks(context.Background(), sess, "hello there", nil, "default/foo", false, noopInfoLogger{}) + assert.Contains(t, concatBlockText(blocks), "hello there", "text renders as a section when no components are present") +} + +// End to end through Send: the post carries Block Kit (Text + Blocks options) +// and the text is the notification fallback. (Block *content* is asserted by +// TestBuildUserMessageBlocks_ComponentsAreTheBody; slack-go does not expose the +// resolved blocks through a MsgOption, so the wire test asserts presence via the +// option shape — the same way the degrade test distinguishes block vs plain-text +// posts.) +func TestSenderUserMessage_ComponentsOnWire(t *testing.T) { + c := &fakeSlackClient{} + s := newSender(c) + sess := channelkinds.SessionInfo{ + Namespace: "default", Name: "foo", + Channel: &spiceboxv1alpha1.ChannelBinding{ + External: map[string]string{"channel_id": "C01ABCDEF"}, + }, + } + components := json.RawMessage(`[{"type":"header","text":{"type":"plain_text","text":"Quarterly Results"}}]`) + _, err := s.Send(context.Background(), sess, userMessageEnvelopeWithComponents(t, "summary fallback", components)) + require.NoError(t, err) + require.Len(t, c.postMessageCalls, 1, "one post") + + opts := c.postMessageCalls[0].options + assert.Len(t, opts, 2, "a components reply posts Text + Blocks (no thread_ts), not a degraded plain-text post") + assert.Equal(t, "summary fallback", renderTextFromOpts(t, opts), "text is the notification fallback") +} + +func mentionOnlySession() channelkinds.SessionInfo { + return channelkinds.SessionInfo{ + Namespace: "default", Name: "foo", + Channel: &spiceboxv1alpha1.ChannelBinding{ + Name: "slack-eng", Kind: "slack", RoutingMode: "mention_only", + External: map[string]string{"channel_id": "C01ABCDEF", "thread_ts": "1614191050.000000"}, + }, + } +} + +var reportComponents = json.RawMessage(`[{"type":"header","text":{"type":"plain_text","text":"Quarterly Results"}}]`) + +// The streaming bubble is the common path in a thread. When components are its +// body, the mention_only hint must render there too — text, which carries it for +// text replies, is only the notification fallback. +func TestSender_StreamingBubble_ComponentsCarryMentionFooter(t *testing.T) { + c := &fakeSlackClient{} + s := newSender(c) + s.streamSink = &stubStreamSink{ts: "1700000000.000111"} + + _, err := s.Send(context.Background(), mentionOnlySession(), + userMessageEnvelopeWithComponents(t, "summary", reportComponents)) + require.NoError(t, err) + require.Len(t, c.updateCalls, 1, "the bubble is updated in place") + + blocks := msgOptionBlocksJSON(t, c.updateCalls[0].opts) + assert.Contains(t, blocks, "Quarterly Results", "the components are the body") + assert.Contains(t, blocks, "@-mention me to reply", "the mention_only hint renders in the bubble") +} + +// In a degraded plain-text retry, the mention_only hint closes the message +// rather than sitting between the summary and the projected components. +func TestSenderUserMessage_DegradedRetry_MentionFooterIsLast(t *testing.T) { + c := &fakeSlackClient{postMessageErrs: []error{slackapi.SlackErrorResponse{Err: "invalid_blocks"}}} + s := newSender(c) + + _, err := s.Send(context.Background(), mentionOnlySession(), + userMessageEnvelopeWithComponents(t, "summary", reportComponents)) + require.NoError(t, err) + require.Len(t, c.postMessageCalls, 2, "block post, then plain-text retry") + + text := renderTextFromOpts(t, c.postMessageCalls[1].options) + assert.Equal(t, "summary\n\nQuarterly Results\n\n_@-mention me to reply._", text, + "summary, then the projected components, then the hint") +} diff --git a/pkg/channels/channelkinds/slack/sender_test.go b/pkg/channels/channelkinds/slack/sender_test.go index 2010d958..69a7b09b 100644 --- a/pkg/channels/channelkinds/slack/sender_test.go +++ b/pkg/channels/channelkinds/slack/sender_test.go @@ -1930,7 +1930,7 @@ func TestSenderTurnProgress_MalformedPayloadReturnsError(t *testing.T) { // GetUserInfoContext is the id→profile direction renderSlackEntities needs. // Seeded via userInfo; an unseeded id returns users_not_found, the same shape // the real API gives for an id from another workspace. -func (f *fakeSlackClient) GetUserInfoContext(_ context.Context, id string) (*slackapi.User, error) { +func (f *fakeSlackClient) GetUserInfoContext(_ context.Context, id string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { f.mu.Lock() defer f.mu.Unlock() f.userInfoCalls = append(f.userInfoCalls, id) diff --git a/pkg/channels/channelkinds/slack/sender_tool_session_test.go b/pkg/channels/channelkinds/slack/sender_tool_session_test.go index 8fba4eda..ff352457 100644 --- a/pkg/channels/channelkinds/slack/sender_tool_session_test.go +++ b/pkg/channels/channelkinds/slack/sender_tool_session_test.go @@ -309,6 +309,6 @@ func TestToolSessionSender_OrdinarySession_ReportsTheBubbleAsRoot(t *testing.T) s.refs.get("tc-plain").stop() // stop the test goroutine's timer } -func (c *recordingClient) GetUserInfoContext(context.Context, string) (*slackapi.User, error) { +func (c *recordingClient) GetUserInfoContext(context.Context, string, ...slackapi.GetUserInfoOption) (*slackapi.User, error) { return nil, nil } diff --git a/pkg/channels/channelkinds/slack/show_settings_test.go b/pkg/channels/channelkinds/slack/show_settings_test.go index f36e5c39..59adb04c 100644 --- a/pkg/channels/channelkinds/slack/show_settings_test.go +++ b/pkg/channels/channelkinds/slack/show_settings_test.go @@ -261,11 +261,36 @@ func TestBuildUserMessageBlocks_SettingsButton_FirstMessageOnly(t *testing.T) { } // no K8sClient: clamp line skipped sess := sessionInfoForTest("default", "my-session") - first := blocksJSON(t, s.buildUserMessageBlocks(noopCtx(t), sess, "hello", "default/my-session", true, noopLogger{})) + firstBlocks, firstAppended := s.buildUserMessageBlocks(noopCtx(t), sess, "hello", nil, "default/my-session", true, noopLogger{}) + first := blocksJSON(t, firstBlocks) assert.Contains(t, first, showSettingsActionID, "first message carries the button") + assert.True(t, firstAppended, "button reported as appended on the first message") - later := blocksJSON(t, s.buildUserMessageBlocks(noopCtx(t), sess, "again", "default/my-session", false, noopLogger{})) + laterBlocks, laterAppended := s.buildUserMessageBlocks(noopCtx(t), sess, "again", nil, "default/my-session", false, noopLogger{}) + later := blocksJSON(t, laterBlocks) assert.NotContains(t, later, showSettingsActionID, "subsequent messages must not carry the button") + assert.False(t, laterAppended, "button reported as not appended when includeSettings is false") +} + +// TestBuildUserMessageBlocks_SettingsButton_SuppressedWhenNoHeadroom: a 50-block +// agent body leaves no room for the first-message settings button. The function +// must report it was NOT appended (so Send does not consume the one-shot and the +// button appears on a later reply with room) and the button must be absent. +func TestBuildUserMessageBlocks_SettingsButton_SuppressedWhenNoHeadroom(t *testing.T) { + s := &slackSender{statusCache: make(map[string]cachedStatus)} // no K8sClient: clamp line skipped + sess := sessionInfoForTest("default", "my-session") + + dividers := make([]map[string]string, slackMaxBlocksPerMessage) + for i := range dividers { + dividers[i] = map[string]string{"type": "divider"} + } + components, err := json.Marshal(dividers) + require.NoError(t, err, "marshal 50 divider blocks") + + blocks, settingsAppended := s.buildUserMessageBlocks(noopCtx(t), sess, "summary", components, "default/my-session", true, noopLogger{}) + assert.False(t, settingsAppended, "button not appended when the 50-block body leaves no headroom") + assert.Len(t, blocks, slackMaxBlocksPerMessage, "body fills the per-message block cap") + assert.False(t, hasActionBlockWithID(blocks, showSettingsActionID), "no settings button in a full body") } // TestSettingsButtonSeen_OncePerSession: the shared seen-set hands out @@ -287,20 +312,23 @@ func TestSettingsButtonSeen_OncePerSession(t *testing.T) { // // Observation strategy: the Slack SDK's MsgOptionBlocks content is not // recoverable from a captured MsgOption, so the button cannot be read back off -// the posted message. But Send() gates BOTH the button and the seen-set -// mark() on the SAME includeSettings value: +// the posted message. But Send() gates the seen-set mark() on whether +// buildUserMessageBlocks actually APPENDED the button AND the block post +// succeeded: // // includeSettings := s.settingsSeen == nil || !s.settingsSeen.saw(sessRef) -// blocks := s.buildUserMessageBlocks(..., includeSettings, ...) +// blocks, settingsAppended := s.buildUserMessageBlocks(..., includeSettings, ...) // ...post... -// if includeSettings && usedBlocks && s.settingsSeen != nil { s.settingsSeen.mark(sessRef) } +// if settingsAppended && usedBlocks && s.settingsSeen != nil { s.settingsSeen.mark(sessRef) } // // So `seen.saw(sessRef)` flipping to true after a Send is equivalent to -// "includeSettings was true (button built into the blocks) AND the block -// post succeeded" — making the seen-set a faithful proxy for "the button -// was delivered on this send." That closes the loop with -// TestBuildUserMessageBlocks_SettingsButton_FirstMessageOnly, which -// proves includeSettings true⇒button / false⇒no-button. +// "the button was appended into the blocks AND the block post succeeded" — +// making the seen-set a faithful proxy for "the button was delivered on this +// send." That closes the loop with +// TestBuildUserMessageBlocks_SettingsButton_FirstMessageOnly (includeSettings +// true⇒button / false⇒no-button) and +// TestBuildUserMessageBlocks_SettingsButton_SuppressedWhenNoHeadroom (a full +// body appends no button, so the one-shot stays unconsumed). // TestSend_SettingsButton_MarksOnlyAfterSuccessfulBlockPost verifies the // once-guard is consumed only when the button was actually delivered: @@ -399,7 +427,7 @@ func TestBuildUserMessageBlocks_ClampContextBlock_Present(t *testing.T) { statusCache: make(map[string]cachedStatus), } sess := sessionInfoForTest("default", "my-session") - blocks := s.buildUserMessageBlocks(noopCtx(t), sess, "Hello!", "default/my-session", true, noopLogger{}) + blocks, _ := s.buildUserMessageBlocks(noopCtx(t), sess, "Hello!", nil, "default/my-session", true, noopLogger{}) txt := blocksText(blocks) assert.Contains(t, txt, "Budget capped by policy", @@ -423,7 +451,7 @@ func TestBuildUserMessageBlocks_ClampContextBlock_Absent(t *testing.T) { statusCache: make(map[string]cachedStatus), } sess := sessionInfoForTest("default", "my-session") - blocks := s.buildUserMessageBlocks(noopCtx(t), sess, "Hello!", "default/my-session", true, noopLogger{}) + blocks, _ := s.buildUserMessageBlocks(noopCtx(t), sess, "Hello!", nil, "default/my-session", true, noopLogger{}) txt := blocksText(blocks) assert.NotContains(t, txt, "Budget capped by policy", @@ -441,7 +469,7 @@ func TestBuildUserMessageBlocks_ClampLine_BestEffort_OnGetError(t *testing.T) { } sess := sessionInfoForTest("default", "nonexistent-session") // Must not panic or return an error. - blocks := s.buildUserMessageBlocks(noopCtx(t), sess, "Hello!", "default/nonexistent-session", true, noopLogger{}) + blocks, _ := s.buildUserMessageBlocks(noopCtx(t), sess, "Hello!", nil, "default/nonexistent-session", true, noopLogger{}) // Button still present; no clamp line. assert.True(t, hasActionBlockWithID(blocks, showSettingsActionID)) diff --git a/pkg/channels/channelkinds/slack/testhooks_test.go b/pkg/channels/channelkinds/slack/testhooks_test.go index 57774d03..09be70ad 100644 --- a/pkg/channels/channelkinds/slack/testhooks_test.go +++ b/pkg/channels/channelkinds/slack/testhooks_test.go @@ -35,7 +35,7 @@ func (*stubClient) PostMessageContext(_ context.Context, channelID string, _ ... func (*stubClient) PostEphemeralContext(context.Context, string, string, ...slackapi.MsgOption) (string, error) { return "", nil } -func (*stubClient) GetUserInfoContext(_ context.Context, user string) (*slackapi.User, error) { +func (*stubClient) GetUserInfoContext(_ context.Context, user string, _ ...slackapi.GetUserInfoOption) (*slackapi.User, error) { return &slackapi.User{ID: user}, nil } func (*stubClient) OpenViewContext(context.Context, string, slackapi.ModalViewRequest) (*slackapi.ViewResponse, error) {