From b941465bf1ee495d0c0984bda79070a02c80c552 Mon Sep 17 00:00:00 2001 From: MK Date: Thu, 14 May 2026 17:43:54 -0400 Subject: [PATCH] feat(slack): admit bot @mentions via allow_bot_ids, drop self always (closes #55) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Slack adapter previously dropped every event with bot_id != "", blocking legitimate integrations (scheduler, monitoring, workflow, CI bots @mentioning the agent) while a human posting the same text would trigger a response. Lifting that filter wholesale would let the agent respond to its own messages and hot-loop. This change does two things, both inside the slack adapter: - Capture the agent's own bot_id at startup. resolveBotID already called auth.test to get user_id; the same response payload includes bot_id. Store it as p.ownBotID. - Replace the binary filter with admitBotEvent(botID): humans always admitted; the agent's own bot_id is dropped unconditionally (self- loop guard, no opt-out, applies even if listed in allow_bot_ids); every other bot is admitted only when its bot_id appears in the new allow_bot_ids setting on slack-config.yaml. Default behavior is preserved — with allow_bot_ids absent, no other bots are admitted. Admitted bots still have to include <@FORGE_AGENT> in the text to trigger a response (the existing mention-matching block at slack.go:346-361 is untouched), so an admitted scheduler bot posting general chatter does not invoke the agent. Both drop paths emit an operator-actionable log line naming the bot_id and pointing at the YAML setting. Tests: - TestResolveBotID extended: bot_id from auth.test lands on p.ownBotID. - TestParseAllowBotIDs: empty / single / spaces / empty entries. - TestInit_PopulatesAllowBotIDs, TestInit_AllowBotIDsAbsent: setting flows through Init. - TestAdmitBotEvent (table-driven): human, own bot, allowlisted, non-allowlisted; reason includes "allow_bot_ids". - TestAdmitBotEvent_SelfGuardBeatsAllowlist: hard rule from #55 — even if own bot_id is misconfigured into allow_bot_ids, the self-loop guard short-circuits. Template forge-cli/templates/init/slack-config.yaml.tmpl gets a commented example of allow_bot_ids so `forge init` users can discover the option without reading the source. --- .../templates/init/slack-config.yaml.tmpl | 9 ++ forge-plugins/channels/slack/slack.go | 75 +++++++++-- forge-plugins/channels/slack/slack_test.go | 122 +++++++++++++++++- 3 files changed, 192 insertions(+), 14 deletions(-) diff --git a/forge-cli/templates/init/slack-config.yaml.tmpl b/forge-cli/templates/init/slack-config.yaml.tmpl index 620196f..da8da1a 100644 --- a/forge-cli/templates/init/slack-config.yaml.tmpl +++ b/forge-cli/templates/init/slack-config.yaml.tmpl @@ -2,3 +2,12 @@ adapter: slack settings: app_token_env: SLACK_APP_TOKEN bot_token_env: SLACK_BOT_TOKEN + + # Optional: comma-separated list of bot_ids whose @mentions of this agent + # should be honored. By default no other bots are admitted, so the agent + # only responds to humans. Use this to let a scheduler, monitoring tool, + # or CI bot mention the agent. The agent's own bot_id is always dropped + # regardless of this list (self-loop guard). + # + # Find a bot's bot_id: Slack admin → Manage apps → app → Bot User OAuth. + # allow_bot_ids: B0123ABC,B0456DEF diff --git a/forge-plugins/channels/slack/slack.go b/forge-plugins/channels/slack/slack.go index 9b476cc..f474383 100644 --- a/forge-plugins/channels/slack/slack.go +++ b/forge-plugins/channels/slack/slack.go @@ -29,16 +29,18 @@ const longRunningThreshold = 15 * time.Second // Plugin implements channels.ChannelPlugin for Slack using Socket Mode. type Plugin struct { - appToken string - botToken string - botUserID string // resolved at startup via auth.test - wsConn *websocket.Conn - connMu sync.Mutex - stopCh chan struct{} - client *http.Client - apiBase string // overridable for tests - dedupMu sync.Mutex - dedupCache map[string]time.Time + appToken string + botToken string + botUserID string // resolved at startup via auth.test + ownBotID string // resolved at startup via auth.test; used as the self-loop guard + allowBotIDs map[string]bool // bot_ids whose @mentions are admitted; default empty (no other bots admitted) + wsConn *websocket.Conn + connMu sync.Mutex + stopCh chan struct{} + client *http.Client + apiBase string // overridable for tests + dedupMu sync.Mutex + dedupCache map[string]time.Time } // New creates an uninitialised Slack plugin. @@ -64,10 +66,52 @@ func (p *Plugin) Init(cfg channels.ChannelConfig) error { return fmt.Errorf("slack: bot_token is required (set SLACK_BOT_TOKEN)") } + // Optional: comma-separated list of bot_ids whose @mentions of the agent + // are admitted. Empty (the default) means no other bots are admitted — + // the agent only responds to humans. The agent's own bot_id is always + // dropped regardless of this list (see ownBotID guard in Start). + p.allowBotIDs = parseAllowBotIDs(settings["allow_bot_ids"]) + return nil } -// resolveBotID calls auth.test to discover the bot's own Slack user ID. +// parseAllowBotIDs splits a comma-separated bot_id list into a lookup set. +// Whitespace around entries is trimmed; empty entries are skipped. +func parseAllowBotIDs(raw string) map[string]bool { + set := make(map[string]bool) + for _, id := range strings.Split(raw, ",") { + id = strings.TrimSpace(id) + if id != "" { + set[id] = true + } + } + return set +} + +// admitBotEvent decides whether an inbound event authored by a bot should +// flow through to the agent. Human messages (botID == "") always admit. +// The agent's own bot_id is dropped unconditionally — this is the self-loop +// guard, not subject to the allowlist. Any other bot is admitted only when +// its bot_id appears in allowBotIDs. +// +// The returned reason string is the operator-facing log line for dropped +// events; for admitted events it is empty. +func (p *Plugin) admitBotEvent(botID string) (reason string, admit bool) { + if botID == "" { + return "", true + } + if botID == p.ownBotID { + return fmt.Sprintf("dropping event authored by self (bot_id=%s)", botID), false + } + if !p.allowBotIDs[botID] { + return fmt.Sprintf("dropping event from non-allowlisted bot (bot_id=%s); add to slack-config.yaml allow_bot_ids to admit", botID), false + } + return "", true +} + +// resolveBotID calls auth.test to discover the bot's own Slack user ID and +// bot_id. The user ID drives @mention matching; the bot_id powers the +// self-loop guard that drops messages authored by this same bot. func (p *Plugin) resolveBotID() error { req, err := http.NewRequest(http.MethodPost, p.apiBase+"/auth.test", nil) if err != nil { @@ -90,6 +134,7 @@ func (p *Plugin) resolveBotID() error { var result struct { OK bool `json:"ok"` UserID string `json:"user_id"` + BotID string `json:"bot_id"` Error string `json:"error,omitempty"` } if err := json.Unmarshal(body, &result); err != nil { @@ -100,6 +145,7 @@ func (p *Plugin) resolveBotID() error { } p.botUserID = result.UserID + p.ownBotID = result.BotID return nil } @@ -325,8 +371,11 @@ func (p *Plugin) readLoop(ctx context.Context, conn *websocket.Conn, handler cha continue } - // Skip bot messages. - if payload.Event.BotID != "" { + // Bot-authored events go through admitBotEvent: self-mentions are + // always dropped (loop guard); other bots are dropped unless the + // operator has admitted them via allow_bot_ids in slack-config.yaml. + if reason, admit := p.admitBotEvent(payload.Event.BotID); !admit { + fmt.Printf(" slack: %s\n", reason) continue } diff --git a/forge-plugins/channels/slack/slack_test.go b/forge-plugins/channels/slack/slack_test.go index 7d11ee3..0a58722 100644 --- a/forge-plugins/channels/slack/slack_test.go +++ b/forge-plugins/channels/slack/slack_test.go @@ -467,7 +467,9 @@ func TestResolveBotID(t *testing.T) { t.Errorf("Authorization = %q, want 'Bearer xoxb-test-token'", r.Header.Get("Authorization")) } w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"ok":true,"user_id":"U123BOT"}`)) //nolint:errcheck + // auth.test returns both user_id and bot_id; the plugin needs both — + // user_id for @mention matching, bot_id for the self-loop guard. + w.Write([]byte(`{"ok":true,"user_id":"U123BOT","bot_id":"B123BOT"}`)) //nolint:errcheck })) defer srv.Close() @@ -482,6 +484,9 @@ func TestResolveBotID(t *testing.T) { if p.botUserID != "U123BOT" { t.Errorf("botUserID = %q, want U123BOT", p.botUserID) } + if p.ownBotID != "B123BOT" { + t.Errorf("ownBotID = %q, want B123BOT", p.ownBotID) + } } func TestResolveBotID_Error(t *testing.T) { @@ -788,3 +793,118 @@ func TestEvictExpiredDedup(t *testing.T) { t.Error("recent-env should still be present") } } + +// --- Bot-mention admission (issue #55) ----------------------------------- + +func TestParseAllowBotIDs(t *testing.T) { + tests := []struct { + name string + raw string + want []string // sorted list of ids expected in the set + }{ + {"empty", "", nil}, + {"single", "B0123ABC", []string{"B0123ABC"}}, + {"two with spaces", " B0123ABC , B0456DEF ", []string{"B0123ABC", "B0456DEF"}}, + {"empty entries ignored", "B0123ABC,,B0456DEF,", []string{"B0123ABC", "B0456DEF"}}, + {"whitespace only ignored", " , , ", nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := parseAllowBotIDs(tt.raw) + if len(got) != len(tt.want) { + t.Fatalf("got %d entries, want %d (entries=%v)", len(got), len(tt.want), got) + } + for _, id := range tt.want { + if !got[id] { + t.Errorf("missing %q in result", id) + } + } + }) + } +} + +func TestInit_PopulatesAllowBotIDs(t *testing.T) { + p := New() + err := p.Init(channels.ChannelConfig{ + Adapter: "slack", + Settings: map[string]string{ + "app_token": "xoxa-test", + "bot_token": "xoxb-test", + "allow_bot_ids": "B0123ABC, B0456DEF", + }, + }) + if err != nil { + t.Fatalf("Init: %v", err) + } + if !p.allowBotIDs["B0123ABC"] || !p.allowBotIDs["B0456DEF"] { + t.Errorf("allowBotIDs = %v, want both B0123ABC and B0456DEF admitted", p.allowBotIDs) + } +} + +func TestInit_AllowBotIDsAbsent(t *testing.T) { + // Default behavior: with no allow_bot_ids setting, the allowlist is + // empty and only humans (botID == "") flow through admitBotEvent. + p := New() + err := p.Init(channels.ChannelConfig{ + Adapter: "slack", + Settings: map[string]string{ + "app_token": "xoxa-test", + "bot_token": "xoxb-test", + }, + }) + if err != nil { + t.Fatalf("Init: %v", err) + } + if len(p.allowBotIDs) != 0 { + t.Errorf("expected empty allowBotIDs, got %v", p.allowBotIDs) + } +} + +func TestAdmitBotEvent(t *testing.T) { + p := New() + p.ownBotID = "B0SELF" + p.allowBotIDs = map[string]bool{"B0ALLOWED": true} + + tests := []struct { + name string + botID string + wantAdmit bool + // wantReasonSubstr is checked when admit is false to make sure the + // log line is operator-actionable. + wantReasonSubstr string + }{ + {"human message admitted", "", true, ""}, + {"own bot_id dropped (self-loop guard)", "B0SELF", false, "authored by self"}, + {"allowlisted bot admitted", "B0ALLOWED", true, ""}, + {"non-allowlisted bot dropped", "B0OTHER", false, "non-allowlisted"}, + {"non-allowlisted bot reason mentions allow_bot_ids", "B0OTHER", false, "allow_bot_ids"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reason, admit := p.admitBotEvent(tt.botID) + if admit != tt.wantAdmit { + t.Errorf("admit = %v, want %v (reason=%q)", admit, tt.wantAdmit, reason) + } + if !admit && !strings.Contains(reason, tt.wantReasonSubstr) { + t.Errorf("reason = %q, want substring %q", reason, tt.wantReasonSubstr) + } + }) + } +} + +// TestAdmitBotEvent_SelfGuardBeatsAllowlist verifies the hard rule from #55: +// even if the agent's own bot_id were somehow listed in allow_bot_ids, +// the self-loop guard short-circuits before the allowlist check. +func TestAdmitBotEvent_SelfGuardBeatsAllowlist(t *testing.T) { + p := New() + p.ownBotID = "B0SELF" + p.allowBotIDs = map[string]bool{"B0SELF": true} // misconfiguration + + reason, admit := p.admitBotEvent("B0SELF") + if admit { + t.Fatal("agent must never admit its own bot_id, even when allowlisted") + } + if !strings.Contains(reason, "self") { + t.Errorf("reason should identify self-loop guard, got %q", reason) + } +}