diff --git a/api/v1alpha2/taskspawner_types.go b/api/v1alpha2/taskspawner_types.go index c6c49da8..0c74739c 100644 --- a/api/v1alpha2/taskspawner_types.go +++ b/api/v1alpha2/taskspawner_types.go @@ -649,7 +649,11 @@ type GenericWebhookFilter struct { // are configured on the server, not per-TaskSpawner. // // The bot must be invited to each channel it should listen in; the Channels -// field is a post-delivery filter, not a privacy scope. +// and ExcludeChannels fields are post-delivery filters, not a privacy scope. +// The server has already received the message — and, for a thread reply, has +// already fetched the thread history — before either field is consulted, and +// the bot stays in an excluded channel and still greets it on join. Remove the +// bot from a channel to stop delivery itself. // // Bot mention (@bot) is implicitly required by default. The handler knows its // own bot user ID from the Slack auth response. When Triggers are configured, @@ -665,6 +669,25 @@ type Slack struct { // +kubebuilder:validation:items:Pattern=`^[CG][A-Z0-9]{8,}$` Channels []string `json:"channels,omitempty"` + // ExcludeChannels rejects Slack events from the given channels regardless + // of the Channels allowlist — an excluded channel is never matched, even + // when Channels is empty (all channels) or names the same channel. + // Unlike ExcludePatterns, this also applies to slash commands. + // + // Values are channel IDs. Direct-message IDs ("D0123456789") are accepted + // here even though Channels does not accept them, so a spawner that + // listens in every channel can still be kept out of DMs. + // + // The exclusion is only guaranteed while the object is managed through + // v1alpha2. This field does not exist in v1alpha1; it survives a v1alpha1 + // round-trip through a preservation annotation, so a v1alpha1 client that + // drops unknown annotations drops the exclusion with them. + // +optional + // +listType=set + // +kubebuilder:validation:MaxItems=64 + // +kubebuilder:validation:items:Pattern=`^[CGD][A-Z0-9]{8,}$` + ExcludeChannels []string `json:"excludeChannels,omitempty"` + // BotMessages controls whether bot-originated messages can trigger this // spawner. Accepting bot messages carries loop risk — especially "All" // which includes the bot's own output. Use ExcludePatterns or Triggers diff --git a/api/v1alpha2/zz_generated.deepcopy.go b/api/v1alpha2/zz_generated.deepcopy.go index e0f983d5..7e492c67 100644 --- a/api/v1alpha2/zz_generated.deepcopy.go +++ b/api/v1alpha2/zz_generated.deepcopy.go @@ -1405,6 +1405,11 @@ func (in *Slack) DeepCopyInto(out *Slack) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.ExcludeChannels != nil { + in, out := &in.ExcludeChannels, &out.ExcludeChannels + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Triggers != nil { in, out := &in.Triggers, &out.Triggers *out = make([]SlackTrigger, len(*in)) diff --git a/docs/reference.md b/docs/reference.md index 69d863f4..ee558bbe 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -813,6 +813,7 @@ to receive refreshed credentials during long-running work. | `spec.when.linearWebhook.filters[].excludeLabels` | Exclude issues with any of these labels | No | | `spec.when.linearWebhook.gatewayRef.name` | Bind this source to a [WebhookGateway](#webhookgateway) in the same namespace whose `spec.linear` field is set. The per-source webhook server ignores this spawner when the reference is present | No | | `spec.when.slack.channels` | Restrict which Slack channels the bot listens in (channel IDs like `"C0123456789"`); when empty, listens in all invited channels | No | +| `spec.when.slack.excludeChannels` | Channel IDs this spawner never matches; exclusion always wins, so a channel listed here is rejected even when `channels` is empty (all channels) or names the same channel. Unlike `excludePatterns`, it also applies to slash commands. Direct-message IDs (`"D0123456789"`) are accepted here even though `channels` does not accept them. Filters after delivery, like `channels` — the bot stays in the channel. Stored only in `v1alpha2`; a client that writes the spawner through `v1alpha1` preserves the exclusion in an annotation, so stripping that annotation drops it | No | | `spec.when.slack.botMessagePolicy` | Controls whether bot-originated messages can trigger this spawner: `None` (default) rejects all bot messages, `All` allows all including self, `OthersOnly` allows other bots but rejects the bot's own output to prevent self-trigger loops | No | | `spec.when.slack.triggers[].pattern` | RE2 regex matched against message text (unanchored); leading `<@USER_ID>` mentions are stripped before matching; bot mention required unless `mentionOptional` is set; multiple triggers use OR semantics; when empty, every bot mention fires | No | | `spec.when.slack.triggers[].mentionOptional` | When `true`, fire on pattern match alone without requiring a bot @-mention | No | diff --git a/internal/cli/printer.go b/internal/cli/printer.go index 859ede33..210f9ffa 100644 --- a/internal/cli/printer.go +++ b/internal/cli/printer.go @@ -324,6 +324,9 @@ func printTaskSpawnerDetail(w io.Writer, ts *kelos.TaskSpawner) { if len(sl.Channels) > 0 { printField(w, "Channels", fmt.Sprintf("%v", sl.Channels)) } + if len(sl.ExcludeChannels) > 0 { + printField(w, "Exclude Channels", fmt.Sprintf("%v", sl.ExcludeChannels)) + } if len(sl.Triggers) > 0 { patterns := make([]string, len(sl.Triggers)) for i, tr := range sl.Triggers { diff --git a/internal/cli/printer_test.go b/internal/cli/printer_test.go index 3261e698..e4760a70 100644 --- a/internal/cli/printer_test.go +++ b/internal/cli/printer_test.go @@ -447,7 +447,8 @@ func TestPrintTaskSpawnerDetailSlack(t *testing.T) { Spec: kelos.TaskSpawnerSpec{ When: kelos.When{ Slack: &kelos.Slack{ - Channels: []string{"C0123456789", "C9876543210"}, + Channels: []string{"C0123456789", "C9876543210"}, + ExcludeChannels: []string{"C1122334455"}, Triggers: []kelos.SlackTrigger{ {Pattern: "deploy"}, {Pattern: "rollback"}, @@ -473,6 +474,7 @@ func TestPrintTaskSpawnerDetailSlack(t *testing.T) { for _, expected := range []string{ "Source: Slack", "Channels: [C0123456789 C9876543210]", + "Exclude Channels: [C1122334455]", "Triggers: [deploy rollback]", "Exclude Patterns: [^ignore]", } { diff --git a/internal/conversion/taskspawner.go b/internal/conversion/taskspawner.go index 57e610a6..48afe79b 100644 --- a/internal/conversion/taskspawner.go +++ b/internal/conversion/taskspawner.go @@ -3,6 +3,7 @@ package conversion import ( "context" "encoding/json" + "regexp" v1alpha1 "github.com/kelos-dev/kelos/api/v1alpha1" v1alpha2 "github.com/kelos-dev/kelos/api/v1alpha2" @@ -42,6 +43,21 @@ type preservedWebhookGatewayRefs struct { Generic *v1alpha2.GatewayReference `json:"generic,omitempty"` } +// preservedSlackExcludeChannelsAnnotation carries spec.when.slack.excludeChannels +// (a v1alpha2-only field) across a v1alpha1 round-trip so a client that reads +// and writes the object through v1alpha1 does not silently drop it. v1alpha1 +// does not gain the capability — the value only survives in this annotation. +const preservedSlackExcludeChannelsAnnotation = "kelos.dev/v1alpha2-slack-exclude-channels" + +// slackExcludeChannelsMaxItems and slackExcludeChannelIDPattern mirror the +// validation markers on v1alpha2 Slack.ExcludeChannels. The API server does not +// re-validate the output of a conversion webhook, so annotation data — which any +// v1alpha1 client can write by hand — would otherwise reach the hub object +// having bypassed the field's own constraints. +const slackExcludeChannelsMaxItems = 64 + +var slackExcludeChannelIDPattern = regexp.MustCompile(`^[CGD][A-Z0-9]{8,}$`) + type preservedGitHubCommentsReporting struct { GitHubIssues *preservedGitHubCommentsSource `json:"githubIssues,omitempty"` GitHubPullRequests *preservedGitHubCommentsSource `json:"githubPullRequests,omitempty"` @@ -76,6 +92,8 @@ func taskSpawnerToHub(_ context.Context, src *v1alpha1.TaskSpawner, dst *v1alpha deleteAnnotation(dst.Annotations, preservedGitHubCommentsReportingAnnotation) restorePreservedWebhookGatewayRefs(src.Annotations, &dst.Spec.When) deleteAnnotation(dst.Annotations, preservedWebhookGatewayRefsAnnotation) + restorePreservedSlackExcludeChannels(src.Annotations, dst.Spec.When.Slack) + deleteAnnotation(dst.Annotations, preservedSlackExcludeChannelsAnnotation) return nil } @@ -101,6 +119,9 @@ func taskSpawnerFromHub(_ context.Context, src *v1alpha2.TaskSpawner, dst *v1alp if err := setPreservedWebhookGatewayRefs(dst, src.Spec.When); err != nil { return err } + if err := setPreservedSlackExcludeChannels(dst, src.Spec.When.Slack); err != nil { + return err + } return convertViaJSON(&src.Status, &dst.Status) } @@ -170,6 +191,71 @@ func restorePreservedNameTemplate(annotations map[string]string, dst *v1alpha2.T } } +// setPreservedSlackExcludeChannels records spec.when.slack.excludeChannels in +// an annotation on the v1alpha1 object so the field survives a v1alpha1 +// round-trip. The annotation is cleared when there is nothing to preserve. +func setPreservedSlackExcludeChannels(dst *v1alpha1.TaskSpawner, slack *v1alpha2.Slack) error { + if slack == nil || len(slack.ExcludeChannels) == 0 { + deleteAnnotation(dst.Annotations, preservedSlackExcludeChannelsAnnotation) + return nil + } + data, err := json.Marshal(slack.ExcludeChannels) + if err != nil { + return err + } + if dst.Annotations == nil { + dst.Annotations = map[string]string{} + } + dst.Annotations[preservedSlackExcludeChannelsAnnotation] = string(data) + return nil +} + +// restorePreservedSlackExcludeChannels restores excludeChannels dropped by a +// v1alpha1 round-trip, unless the v1alpha2 object already carries the field. +func restorePreservedSlackExcludeChannels(annotations map[string]string, slack *v1alpha2.Slack) { + if slack == nil || len(slack.ExcludeChannels) > 0 { + return + } + raw, ok := annotations[preservedSlackExcludeChannelsAnnotation] + if !ok || raw == "" { + return + } + var excludeChannels []string + if err := json.Unmarshal([]byte(raw), &excludeChannels); err != nil || len(excludeChannels) == 0 { + // The annotation is best-effort preservation data and can be set by + // users; malformed data must not block API version conversion. + return + } + if !validSlackExcludeChannels(excludeChannels) { + return + } + slack.ExcludeChannels = excludeChannels +} + +// validSlackExcludeChannels reports whether restored annotation data satisfies +// the constraints declared on v1alpha2 Slack.ExcludeChannels: at most +// slackExcludeChannelsMaxItems entries, each a well-formed channel ID, no +// duplicates (the field is a set). Data that fails any of these is treated the +// same as malformed JSON — ignored entirely, rather than partially applied, so +// conversion can never produce a hub object that a v1alpha2 write would have +// rejected. +func validSlackExcludeChannels(excludeChannels []string) bool { + if len(excludeChannels) > slackExcludeChannelsMaxItems { + return false + } + seen := make(map[string]struct{}, len(excludeChannels)) + for _, id := range excludeChannels { + if !slackExcludeChannelIDPattern.MatchString(id) { + return false + } + if _, dup := seen[id]; dup { + return false + } + seen[id] = struct{}{} + } + return true +} + // setPreservedContextGitHubAppAuth records the githubAppAuth block of each // context source (keyed by source name) into an annotation on the v1alpha1 // object so it survives a v1alpha1 round-trip. The annotation is cleared when diff --git a/internal/conversion/taskspawner_test.go b/internal/conversion/taskspawner_test.go index af6895b8..b3c5c505 100644 --- a/internal/conversion/taskspawner_test.go +++ b/internal/conversion/taskspawner_test.go @@ -2,6 +2,8 @@ package conversion import ( "context" + "encoding/json" + "fmt" "testing" corev1 "k8s.io/api/core/v1" @@ -262,6 +264,191 @@ func TestTaskSpawnerConvert_ModernFieldsRoundTrip(t *testing.T) { } } +// TestTaskSpawnerConvert_SlackExcludeChannelsRoundTrip verifies that the +// v1alpha2-only slack excludeChannels field survives a v1alpha1 round-trip via +// its preservation annotation while shared Slack fields are carried directly. +func TestTaskSpawnerConvert_SlackExcludeChannelsRoundTrip(t *testing.T) { + src := &v1alpha2.TaskSpawner{ + Spec: v1alpha2.TaskSpawnerSpec{ + When: v1alpha2.When{ + Slack: &v1alpha2.Slack{ + Channels: []string{"C0123456789"}, + ExcludeChannels: []string{"C9876543210", "D0123456789"}, + }, + }, + }, + } + + down := &v1alpha1.TaskSpawner{} + if err := taskSpawnerFromHub(context.Background(), src, down); err != nil { + t.Fatalf("taskSpawnerFromHub() error = %v", err) + } + if down.Spec.When.Slack == nil { + t.Fatal("expected slack config after down-conversion") + } + if len(down.Spec.When.Slack.Channels) != 1 || down.Spec.When.Slack.Channels[0] != "C0123456789" { + t.Errorf("shared channels not preserved: %#v", down.Spec.When.Slack.Channels) + } + // v1alpha1 cannot represent excludeChannels — it survives only via the + // preservation annotation. + if raw, ok := down.Annotations[preservedSlackExcludeChannelsAnnotation]; !ok || raw != `["C9876543210","D0123456789"]` { + t.Errorf("preservation annotation = %q, want the excludeChannels JSON", raw) + } + + up := &v1alpha2.TaskSpawner{} + if err := taskSpawnerToHub(context.Background(), down, up); err != nil { + t.Fatalf("taskSpawnerToHub() error = %v", err) + } + if up.Spec.When.Slack == nil { + t.Fatal("expected slack config after up-conversion") + } + got := up.Spec.When.Slack.ExcludeChannels + if len(got) != 2 || got[0] != "C9876543210" || got[1] != "D0123456789" { + t.Errorf("excludeChannels not restored: %#v", got) + } + if _, ok := up.Annotations[preservedSlackExcludeChannelsAnnotation]; ok { + t.Error("preservation annotation not cleaned up after restore") + } +} + +func TestTaskSpawnerToHub_MalformedSlackExcludeChannelsAnnotationIgnored(t *testing.T) { + // The preservation annotation is user-editable; a malformed value must not + // block conversion to the storage version. It is treated as absent and + // stripped from the hub object so the internal key does not leak into the + // v1alpha2 view. + spoke := &v1alpha1.TaskSpawner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "chat", + Namespace: "default", + Annotations: map[string]string{ + preservedSlackExcludeChannelsAnnotation: "[not valid json", + }, + }, + Spec: v1alpha1.TaskSpawnerSpec{ + When: v1alpha1.When{Slack: &v1alpha1.Slack{Channels: []string{"C0123456789"}}}, + }, + } + + hub := &v1alpha2.TaskSpawner{} + if err := taskSpawnerToHub(context.Background(), spoke, hub); err != nil { + t.Fatalf("taskSpawnerToHub() error = %v", err) + } + if hub.Spec.When.Slack == nil { + t.Fatal("expected slack config after up-conversion") + } + if got := hub.Spec.When.Slack.ExcludeChannels; len(got) != 0 { + t.Errorf("excludeChannels = %#v, want none from a malformed annotation", got) + } + if _, ok := hub.Annotations[preservedSlackExcludeChannelsAnnotation]; ok { + t.Error("malformed preservation annotation should still be stripped from the hub object") + } +} + +// marshalChannelIDs returns a JSON array of n unique, well-formed Slack channel +// IDs, for exercising the maxItems boundary of the preservation annotation. +func marshalChannelIDs(t *testing.T, n int) string { + t.Helper() + ids := make([]string, 0, n) + for i := 0; i < n; i++ { + ids = append(ids, fmt.Sprintf("C%09d", i)) + } + raw, err := json.Marshal(ids) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + return string(raw) +} + +func TestTaskSpawnerToHub_InvalidSlackExcludeChannelsAnnotationIgnored(t *testing.T) { + // The API server does not re-validate conversion output, so annotation data + // that violates the v1alpha2 constraints must not be restored — otherwise a + // v1alpha1 write could plant values the v1alpha2 schema would have rejected. + tests := []struct { + name string + raw string + }{ + {"channel id that fails the item pattern", `["c0123456789"]`}, + {"channel id that is too short", `["C123"]`}, + {"more entries than maxItems allows", marshalChannelIDs(t, slackExcludeChannelsMaxItems+1)}, + {"duplicate entries in a set", `["C0123456789","C0123456789"]`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spoke := &v1alpha1.TaskSpawner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "chat", + Namespace: "default", + Annotations: map[string]string{preservedSlackExcludeChannelsAnnotation: tt.raw}, + }, + Spec: v1alpha1.TaskSpawnerSpec{ + When: v1alpha1.When{Slack: &v1alpha1.Slack{Channels: []string{"C0123456789"}}}, + }, + } + + hub := &v1alpha2.TaskSpawner{} + if err := taskSpawnerToHub(context.Background(), spoke, hub); err != nil { + t.Fatalf("taskSpawnerToHub() error = %v", err) + } + if hub.Spec.When.Slack == nil { + t.Fatal("expected slack config after up-conversion") + } + if got := hub.Spec.When.Slack.ExcludeChannels; len(got) != 0 { + t.Errorf("excludeChannels = %#v, want none from annotation data that violates the field constraints", got) + } + if _, ok := hub.Annotations[preservedSlackExcludeChannelsAnnotation]; ok { + t.Error("invalid preservation annotation should still be stripped from the hub object") + } + }) + } +} + +func TestTaskSpawnerToHub_MaxSlackExcludeChannelsAnnotationRestored(t *testing.T) { + // The boundary case must still restore: exactly maxItems valid, unique IDs. + spoke := &v1alpha1.TaskSpawner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "chat", + Namespace: "default", + Annotations: map[string]string{ + preservedSlackExcludeChannelsAnnotation: marshalChannelIDs(t, slackExcludeChannelsMaxItems), + }, + }, + Spec: v1alpha1.TaskSpawnerSpec{ + When: v1alpha1.When{Slack: &v1alpha1.Slack{}}, + }, + } + + hub := &v1alpha2.TaskSpawner{} + if err := taskSpawnerToHub(context.Background(), spoke, hub); err != nil { + t.Fatalf("taskSpawnerToHub() error = %v", err) + } + if got := hub.Spec.When.Slack.ExcludeChannels; len(got) != slackExcludeChannelsMaxItems { + t.Errorf("excludeChannels length = %d, want %d", len(got), slackExcludeChannelsMaxItems) + } +} + +func TestTaskSpawnerFromHub_NoSlackExcludeChannelsOmitsAnnotation(t *testing.T) { + hub := &v1alpha2.TaskSpawner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "chat", + Namespace: "default", + Annotations: map[string]string{ + preservedSlackExcludeChannelsAnnotation: `["C9876543210"]`, + }, + }, + Spec: v1alpha2.TaskSpawnerSpec{ + When: v1alpha2.When{Slack: &v1alpha2.Slack{Channels: []string{"C0123456789"}}}, + }, + } + spoke := &v1alpha1.TaskSpawner{} + if err := taskSpawnerFromHub(context.Background(), hub, spoke); err != nil { + t.Fatalf("taskSpawnerFromHub() error = %v", err) + } + if _, ok := spoke.Annotations[preservedSlackExcludeChannelsAnnotation]; ok { + t.Error("annotation should be cleared when excludeChannels is empty") + } +} + // TestTaskSpawnerConvert_CheckRunFilterFieldsDownConvert verifies that the // v1alpha2-only check_run filter fields (Conclusion, CheckName) convert down to // v1alpha1 without error. v1alpha1 has no equivalent fields, so they are dropped diff --git a/internal/manifests/charts/kelos/charts/kelos-crds/templates/taskspawner-crd.yaml b/internal/manifests/charts/kelos/charts/kelos-crds/templates/taskspawner-crd.yaml index d184e432..50f3a46b 100644 --- a/internal/manifests/charts/kelos/charts/kelos-crds/templates/taskspawner-crd.yaml +++ b/internal/manifests/charts/kelos/charts/kelos-crds/templates/taskspawner-crd.yaml @@ -23317,6 +23317,27 @@ spec: type: string maxItems: 64 type: array + excludeChannels: + description: |- + ExcludeChannels rejects Slack events from the given channels regardless + of the Channels allowlist — an excluded channel is never matched, even + when Channels is empty (all channels) or names the same channel. + Unlike ExcludePatterns, this also applies to slash commands. + + Values are channel IDs. Direct-message IDs ("D0123456789") are accepted + here even though Channels does not accept them, so a spawner that + listens in every channel can still be kept out of DMs. + + The exclusion is only guaranteed while the object is managed through + v1alpha2. This field does not exist in v1alpha1; it survives a v1alpha1 + round-trip through a preservation annotation, so a v1alpha1 client that + drops unknown annotations drops the exclusion with them. + items: + pattern: ^[CGD][A-Z0-9]{8,}$ + type: string + maxItems: 64 + type: array + x-kubernetes-list-type: set excludePatterns: description: |- ExcludePatterns rejects messages whose text matches any of the given diff --git a/internal/manifests/install-crd.yaml b/internal/manifests/install-crd.yaml index e5d8fe6f..f21dda55 100644 --- a/internal/manifests/install-crd.yaml +++ b/internal/manifests/install-crd.yaml @@ -60620,6 +60620,27 @@ spec: type: string maxItems: 64 type: array + excludeChannels: + description: |- + ExcludeChannels rejects Slack events from the given channels regardless + of the Channels allowlist — an excluded channel is never matched, even + when Channels is empty (all channels) or names the same channel. + Unlike ExcludePatterns, this also applies to slash commands. + + Values are channel IDs. Direct-message IDs ("D0123456789") are accepted + here even though Channels does not accept them, so a spawner that + listens in every channel can still be kept out of DMs. + + The exclusion is only guaranteed while the object is managed through + v1alpha2. This field does not exist in v1alpha1; it survives a v1alpha1 + round-trip through a preservation annotation, so a v1alpha1 client that + drops unknown annotations drops the exclusion with them. + items: + pattern: ^[CGD][A-Z0-9]{8,}$ + type: string + maxItems: 64 + type: array + x-kubernetes-list-type: set excludePatterns: description: |- ExcludePatterns rejects messages whose text matches any of the given diff --git a/internal/slack/filter.go b/internal/slack/filter.go index a8f758d9..6130eb46 100644 --- a/internal/slack/filter.go +++ b/internal/slack/filter.go @@ -67,13 +67,15 @@ func getOrCompileRegexp(pattern string) (*regexp.Regexp, error) { } // MatchesSpawner checks whether a Slack message matches the given TaskSpawner's -// Slack configuration (channels, bot mention, trigger patterns, exclude -// patterns, and bot message policy). +// Slack configuration (channels, excluded channels, bot mention, trigger +// patterns, exclude patterns, and bot message policy). Excluded channels are +// rejected before every other check, so — unlike exclude patterns — the +// exclusion also covers slash commands. func MatchesSpawner(slackCfg *kelos.Slack, msg *SlackMessageData, botUserID string) bool { if slackCfg == nil { return false } - if !matchesChannel(msg.ChannelID, slackCfg.Channels) { + if !matchesChannel(msg.ChannelID, slackCfg.Channels, slackCfg.ExcludeChannels) { return false } // Slash commands bypass mention, trigger, and exclude filters. @@ -132,9 +134,16 @@ func ExtractSlackWorkItem(msg *SlackMessageData) map[string]interface{} { } } -// matchesChannel returns true if channelID is in the allowed list, -// or if the allowed list is empty (all channels permitted). -func matchesChannel(channelID string, allowed []string) bool { +// matchesChannel returns false if channelID is in the excluded list, +// otherwise true if channelID is in the allowed list, or if the allowed +// list is empty (all channels permitted). Exclusion always wins over the +// allowlist. +func matchesChannel(channelID string, allowed, excluded []string) bool { + for _, id := range excluded { + if id == channelID { + return false + } + } if len(allowed) == 0 { return true } diff --git a/internal/slack/filter_test.go b/internal/slack/filter_test.go index 34af90f9..95ee6d03 100644 --- a/internal/slack/filter_test.go +++ b/internal/slack/filter_test.go @@ -55,6 +55,64 @@ func TestMatchesSpawner(t *testing.T) { botUserID: "UBOT1", want: false, }, + { + name: "excluded channel rejects even with bot mention", + slackCfg: &kelos.Slack{ + ExcludeChannels: []string{"C1", "C2"}, + }, + msg: &SlackMessageData{UserID: "U1", ChannelID: "C1", Text: "<@UBOT1> hi"}, + botUserID: "UBOT1", + want: false, + }, + { + name: "excluded channel rejects even with matching trigger", + slackCfg: &kelos.Slack{ + ExcludeChannels: []string{"C1"}, + Triggers: []kelos.SlackTrigger{ + {Pattern: "fix.*bug", MentionOptional: boolPtr(true)}, + }, + }, + msg: &SlackMessageData{UserID: "U1", ChannelID: "C1", Text: "fix the bug"}, + botUserID: "UBOT1", + want: false, + }, + { + name: "excluded channel rejects even when allowed list empty", + slackCfg: &kelos.Slack{ + ExcludeChannels: []string{"C1"}, + }, + msg: &SlackMessageData{UserID: "U1", ChannelID: "C1", Text: "<@UBOT1> hi"}, + botUserID: "UBOT1", + want: false, + }, + { + name: "excluded channel rejects slash command", + slackCfg: &kelos.Slack{ + ExcludeChannels: []string{"C1"}, + }, + msg: &SlackMessageData{UserID: "U1", ChannelID: "C1", Text: "/triage something", IsSlashCommand: true}, + botUserID: "UBOT1", + want: false, + }, + { + name: "excluded direct message rejects", + slackCfg: &kelos.Slack{ + ExcludeChannels: []string{"D0123456789"}, + }, + msg: &SlackMessageData{UserID: "U1", ChannelID: "D0123456789", Text: "<@UBOT1> hi"}, + botUserID: "UBOT1", + want: false, + }, + { + name: "non-excluded channel still matches", + slackCfg: &kelos.Slack{ + Channels: []string{"C2", "C3"}, + ExcludeChannels: []string{"C1"}, + }, + msg: &SlackMessageData{UserID: "U1", ChannelID: "C3", Text: "<@UBOT1> hi"}, + botUserID: "UBOT1", + want: true, + }, { name: "trigger with pattern and mention matches", slackCfg: &kelos.Slack{ @@ -487,16 +545,23 @@ func TestMatchesChannel(t *testing.T) { name string channelID string allowed []string + excluded []string want bool }{ - {"empty allowed list matches all", "C1", nil, true}, - {"in allowed list", "C1", []string{"C1", "C2"}, true}, - {"not in allowed list", "C3", []string{"C1", "C2"}, false}, + {"empty allowed list matches all", "C1", nil, nil, true}, + {"in allowed list", "C1", []string{"C1", "C2"}, nil, true}, + {"not in allowed list", "C3", []string{"C1", "C2"}, nil, false}, + {"excluded channel rejects", "C1", nil, []string{"C1"}, false}, + {"excluded channel rejects even when allowed", "C1", []string{"C1"}, []string{"C1"}, false}, + {"excluded channel rejects even when allowed empty", "C1", nil, []string{"C1", "C2"}, false}, + {"non-excluded channel matches when allowed empty", "C3", nil, []string{"C1", "C2"}, true}, + {"non-excluded allowed channel matches", "C2", []string{"C1", "C2"}, []string{"C0"}, true}, + {"excluded direct message rejects", "D1", nil, []string{"D1"}, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := matchesChannel(tt.channelID, tt.allowed); got != tt.want { + if got := matchesChannel(tt.channelID, tt.allowed, tt.excluded); got != tt.want { t.Errorf("matchesChannel() = %v, want %v", got, tt.want) } }) diff --git a/test/integration/conversion_test.go b/test/integration/conversion_test.go index e9ee3e8b..ea5b2cfc 100644 --- a/test/integration/conversion_test.go +++ b/test/integration/conversion_test.go @@ -4,6 +4,7 @@ import ( "encoding/base64" "fmt" "net" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -66,13 +67,25 @@ var _ = Describe("AgentConfig conversion webhook", Ordered, func() { }) AfterAll(func() { - // Remove the AgentConfig objects created here so later install/uninstall - // specs that delete the agentconfigs CRD are not left with instances. + // Remove the objects created here so specs that delete a kelos CRD are + // not left with instances. envtest runs no namespace controller, so the + // namespaces themselves cannot be cleaned up this way — the instances + // have to go individually. for _, ns := range []string{"test-conv-up", "test-conv-down"} { _ = k8sClient.Delete(ctx, &kelos.AgentConfig{ ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: ns}, }) } + // TaskSpawners carry a controller finalizer, so leaving them behind + // would keep instances of the taskspawners CRD alive indefinitely. + for _, ts := range []types.NamespacedName{ + {Name: "ts", Namespace: "test-conv-ts"}, + {Name: "ts-slack", Namespace: "test-conv-ts-slack"}, + } { + _ = k8sClient.Delete(ctx, &kelos.TaskSpawner{ + ObjectMeta: metav1.ObjectMeta{Name: ts.Name, Namespace: ts.Namespace}, + }) + } }) It("converts a v1alpha1 map env up to a v1alpha2 list", func() { @@ -172,4 +185,60 @@ var _ = Describe("AgentConfig conversion webhook", Ordered, func() { Expect(gi.CommentPolicy).NotTo(BeNil()) Expect(gi.CommentPolicy.TriggerComment).To(Equal("/kelos go")) }) + + It("preserves TaskSpawner slack excludeChannels across a v1alpha1 round-trip", func() { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-conv-ts-slack"}} + Expect(k8sClient.Create(ctx, ns)).To(Succeed()) + key := types.NamespacedName{Name: "ts-slack", Namespace: ns.Name} + + By("Creating a v1alpha2 TaskSpawner with slack channels and excludeChannels") + v2 := &kelos.TaskSpawner{ + ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace}, + Spec: kelos.TaskSpawnerSpec{ + When: kelos.When{ + Slack: &kelos.Slack{ + Channels: []string{"C0123456789"}, + ExcludeChannels: []string{"C9876543210", "D0123456789"}, + }, + }, + TaskTemplate: kelos.TaskTemplate{ + Type: "claude-code", + Credentials: &kelos.Credentials{Type: kelos.CredentialTypeNone}, + WorkspaceRef: &kelos.WorkspaceReference{Name: "ws"}, + }, + }, + } + Expect(k8sClient.Create(ctx, v2)).To(Succeed()) + + By("Reading it back as v1alpha1 and asserting the field survives in the preservation annotation") + down := &kelosv1alpha1.TaskSpawner{} + Expect(k8sClient.Get(ctx, key, down)).To(Succeed()) + Expect(down.Spec.When.Slack).NotTo(BeNil()) + Expect(down.Spec.When.Slack.Channels).To(Equal([]string{"C0123456789"})) + Expect(down.Annotations).To(HaveKeyWithValue( + "kelos.dev/v1alpha2-slack-exclude-channels", `["C9876543210","D0123456789"]`)) + + By("Writing the object back through v1alpha1") + // The TaskSpawner controller reconciles this object concurrently, so the + // read-modify-write has to tolerate a conflict on a stale resourceVersion. + Eventually(func() error { + current := &kelosv1alpha1.TaskSpawner{} + if err := k8sClient.Get(ctx, key, current); err != nil { + return err + } + if current.Spec.When.Slack == nil { + return fmt.Errorf("slack config missing on the v1alpha1 read") + } + current.Spec.When.Slack.Channels = []string{"C0123456789", "C1122334455"} + return k8sClient.Update(ctx, current) + }, 10*time.Second, 100*time.Millisecond).Should(Succeed()) + + By("Reading it as v1alpha2 and asserting excludeChannels was restored") + got := &kelos.TaskSpawner{} + Expect(k8sClient.Get(ctx, key, got)).To(Succeed()) + Expect(got.Spec.When.Slack).NotTo(BeNil()) + Expect(got.Spec.When.Slack.Channels).To(Equal([]string{"C0123456789", "C1122334455"})) + Expect(got.Spec.When.Slack.ExcludeChannels).To(Equal([]string{"C9876543210", "D0123456789"})) + Expect(got.Annotations).NotTo(HaveKey("kelos.dev/v1alpha2-slack-exclude-channels")) + }) })