Add Slack as a TaskSpawner source (MVP)#1002
Open
jkahuja wants to merge 6 commits intokelos-dev:mainfrom
Open
Conversation
Adds a new `slack` source type to TaskSpawner that triggers task creation from Slack messages via Socket Mode. A centralized kelos-slack-server connects to Slack via an outbound WebSocket (no ingress required) and routes messages to matching TaskSpawners based on channel restrictions and @-mention requirements. Includes Helm templates for deploying the slack server (Deployment, ServiceAccount, RBAC) and updates the TaskSpawner controller to treat Slack sources as webhook-based (no spawner Deployment created). Reporting (posting status updates back to Slack threads) is deferred to a follow-up PR. Co-Authored-By: Claude Opus 4.6 <[email protected]>
github-actions
Bot
added
kind/feature
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/kelos-slack-server/Dockerfile">
<violation number="1" location="cmd/kelos-slack-server/Dockerfile:1">
P2: Base image is not pinned to an immutable digest/version, so builds can drift over time.</violation>
</file>
<file name="internal/manifests/charts/kelos/templates/rbac.yaml">
<violation number="1" location="internal/manifests/charts/kelos/templates/rbac.yaml:289">
P2: Slack server RBAC grants unnecessary Task update/watch access even though the Slack handler only creates Tasks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The slack handler only creates Tasks — the update and watch verbs were left over from the reporting loop which is not included in this MVP. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Collaborator
|
/kelos api-review |
![kelos-bot[bot]](https://avatars.githubusercontent.com/in/2965305?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
gjkim42
reviewed
Apr 27, 2026
Collaborator
There was a problem hiding this comment.
API Design Review
Verdict: COMMENT
Scope: api/v1alpha1/taskspawner_types.go — Slack source
Desired shape
type Slack struct {
// The bot must be invited to each channel; this is a post-delivery
// filter, not a privacy scope. Empty = every invited channel.
// +optional
// +kubebuilder:validation:MaxItems=64
// +kubebuilder:validation:items:Pattern=`^[CG][A-Z0-9]{8,}$`
Channels []string `json:"channels,omitempty"`
// Bot mention is implicitly required. Pattern triggers add a regex
// AND'd with the mention. Multiple triggers OR. Empty = every bot
// mention fires.
// +optional
// +kubebuilder:validation:MaxItems=8
Triggers []SlackTrigger `json:"triggers,omitempty"`
}
type SlackTrigger struct {
// Go RE2 regex against message text. Unanchored.
// +kubebuilder:validation:Required
// +kubebuilder:validation:MaxLength=256
Pattern string `json:"pattern"`
}Smallest valid spawner:
spec:
when:
slack: {}fires on any bot mention in any invited channel.
gjkim42
requested changes
Apr 27, 2026
Contributor
|
@gjkim42 I like the idea of it listening for itself automatically. But it is also helpful to not require a specific mention, if you want to trigger on some other text then an @ specifically? Might not be worth adding right now? |
Collaborator
|
Then how about adding a field for make it optional? mentionOptional *bool to SlackTrigger? |
Contributor
|
I think that is reasonable |
…er patterns
Reshape the Slack source API per review feedback:
- Remove MentionUserIDs field; bot mention is now implicit (the handler
knows its own user ID from Slack auth)
- Add Triggers []SlackTrigger with RE2 regex patterns (OR semantics)
- Add MentionOptional *bool per trigger to fire on pattern alone
- Add kubebuilder validation markers (MaxItems, Pattern, MaxLength)
Simplest valid config is now `slack: {}` which fires on any bot mention.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/v1alpha1/taskspawner_types.go">
<violation number="1" location="api/v1alpha1/taskspawner_types.go:528">
P1: Slack trigger Pattern field lacks MinLength validation, allowing an empty regex pattern that matches every message and can cause runaway task spawning</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Prevents empty regex patterns that would match every message. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Author
|
@gjkim42 thanks for your feedback! I addressed your comment |
gjkim42
reviewed
Apr 29, 2026
Collaborator
gjkim42
left a comment
gjkim42
left a comment
There was a problem hiding this comment.
you need to sign the CLA.
Allow empty patterns so triggers can match all messages and remain forward-compatible with future trigger conditions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
gjkim42
reviewed
Apr 29, 2026
Collaborator
|
/kelos review |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds Slack as a TaskSpawner source via a centralized kelos-slack-server (Socket Mode) with channel filters, regex triggers, optional bot mention, slash-command handling, and thread-context fetching.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 0 | — | none |
| P2 | 1 | internal/slack/handler.go:233-236 | Top-level messages silently drop attachments — enriched Body is overwritten with msg.Text |
| P3 | 2 | internal/slack/filter.go:86 | Title template variable is the user name, diverges from other sources |
| P3 | internal/slack/filter.go:40-52 | Trigger regexp cache has no eviction policy |
Findings
Correctness
- [P2]
internal/slack/handler.go:233-236—enrichMessagecarefully merges message text andformatAttachments(...)intoSlackMessageData.Body, butrouteMessagethen overridestaskMsg.Body = msg.Textfor any non-thread message, throwing the attachments away beforeExtractSlackWorkItemreadsmsg.Body. The result: a top-level Slack message that says<@bot> investigate thisand forwards another message as an attachment will reach the agent prompt with only theinvestigate thisline — the forwarded content is lost. Either drop the override here (use the enrichedBodyfor top-level messages too) or remove the attachment merging inenrichMessageas dead code. The handler testtop-level message uses raw text as bodyhappens to setText == Bodyso it does not catch this; consider extending it with an attachment-only top-level case.
Code Quality / Suggestions
- [P3]
internal/slack/filter.go:86—ExtractSlackWorkItemmapsTitletomsg.UserName, while other sources (GitHub issue/PR, Jira, Linear) populateTitlewith the actual work-item title. Users templating{{ .Title }}across sources will get inconsistent semantics. Consider mapping to the first line of the message text or documenting the divergence in the godoc onWhen.Slack. (Already raised by the API reviewer agent — re-flagging for visibility, non-blocking.) - [P3]
internal/slack/filter.go:40-52—triggerRegexpCache(async.Map) is populated unconditionally and never evicted. With trigger patterns being user-controlled and TaskSpawners coming and going, the cache grows for the lifetime of the slack-server pod. Bounded byMaxItems=8triggers per spawner * (currently configured + historical) spawners — not an immediate concern but worth a TODO if long-lived deployments churn many spawners.
Key takeaways
- API surface, validation, generated artifacts (CRDs, deepcopy), Helm chart, and webhook-based controller behavior all look consistent with the existing webhook-server pattern; controller test for
isWebhookBasedis appropriately extended. - Logging conventions, RBAC scope (least-privilege after the recent commit dropped
update/watchontasks), least-privilege container security context, and leader-elected Socket Mode runnable look correct. - Only meaningful concern is the top-level message body override that drops attachments; resolve before users hit it in the wild, but it is not a release blocker.
- Make SlackTrigger Pattern optional to allow future trigger conditions - Fix Body override in routeMessage that silently dropped attachments for top-level messages - Map Title to first line of message text instead of UserName for consistency with other sources Co-Authored-By: Claude Opus 4.6 <[email protected]>
Greptile SummaryThis PR adds Slack as a TaskSpawner source via a centralized
Confidence Score: 4/5Two P1 correctness issues in handler.go should be fixed before merging — both can cause silent task loss. The API types, Helm templates, RBAC, filter logic, and controller change are all correct and well-tested. The two P1 issues (thread reply drop on fetch error and task name collision for long spawner names) are straightforward fixes and don't affect the broader architecture. internal/slack/handler.go — thread reply early return and task name truncation logic
|
| Filename | Overview |
|---|---|
| internal/slack/handler.go | Core Socket Mode handler: two P1 issues — thread reply dropped on fetch error instead of falling back, and task name truncation causes collisions for spawner names > 44 chars |
| internal/slack/filter.go | Channel/mention/trigger matching logic; well-tested, correct handling of partial-ID mention detection, empty patterns, and slash command bypass |
| internal/slack/thread.go | Thread context fetching and formatting; BotParticipated is exported but never called in the handler |
| cmd/kelos-slack-server/main.go | Binary entrypoint; correctly sets up controller-runtime manager with leader election and health probes |
| api/v1alpha1/taskspawner_types.go | Adds Slack and SlackTrigger API types with kubebuilder validation markers; deepcopy generated correctly |
| internal/manifests/charts/kelos/templates/slack-server.yaml | Helm Deployment with least-privilege security context, health probes, and secret-backed env vars; image tag interpolation is correct |
| internal/manifests/charts/kelos/templates/rbac.yaml | Least-privilege ClusterRole granting only get/list/watch on taskspawners and create/get/list on tasks |
| internal/controller/taskspawner_controller.go | Correctly treats Slack as webhook-based (no per-spawner Deployment); single-line change with test coverage |
Sequence Diagram
sequenceDiagram
participant Slack
participant SlackHandler
participant Filter
participant TaskBuilder
participant K8s API
Slack->>SlackHandler: Socket Mode event (message / slash command)
SlackHandler->>SlackHandler: Ack event
SlackHandler->>SlackHandler: shouldProcess() filter
SlackHandler->>Slack: GetUserInfoContext + GetPermalinkContext
Slack-->>SlackHandler: user info + permalink
alt Thread reply
SlackHandler->>Slack: GetConversationRepliesContext
Slack-->>SlackHandler: thread messages
SlackHandler->>SlackHandler: FormatThreadContext → msg.Body
end
SlackHandler->>K8s API: List TaskSpawners (all namespaces)
K8s API-->>SlackHandler: TaskSpawnerList
loop For each Slack TaskSpawner
SlackHandler->>Filter: MatchesSpawner(slackCfg, msg, botUserID)
Filter-->>SlackHandler: matched / not matched
alt Matches
SlackHandler->>TaskBuilder: BuildTask(spawner, templateVars)
TaskBuilder-->>SlackHandler: Task object
SlackHandler->>K8s API: Create Task
K8s API-->>SlackHandler: Created / AlreadyExists
end
end
Comments Outside Diff (3)
-
internal/slack/handler.go, line 1424-1432 (link)Thread reply silently dropped on transient fetch failure
If
FetchThreadContextreturns an error (e.g. Slack API rate-limit, transient network error),returnis hit beforeh.routeMessageis called, so the mention is silently dropped — the user gets no task and no feedback. Since thread context is enrichment rather than a hard requirement, it is safer to fall back to the already-enrichedmsg.Bodyand continue routing.if innerEvent.ThreadTimeStamp != "" { body, err := FetchThreadContext(ctx, h.api, innerEvent.Channel, innerEvent.ThreadTimeStamp, h.botUserID) if err != nil { h.log.Error(err, "Failed to fetch thread context, falling back to message text", "channel", innerEvent.Channel, "threadTS", innerEvent.ThreadTimeStamp) // fall through: msg.Body already contains the raw message text } else { msg.Body = body msg.HasThreadContext = true } } h.routeMessage(ctx, msg)
Prompt To Fix With AI
This is a comment left during a code review. Path: internal/slack/handler.go Line: 1424-1432 Comment: **Thread reply silently dropped on transient fetch failure** If `FetchThreadContext` returns an error (e.g. Slack API rate-limit, transient network error), `return` is hit before `h.routeMessage` is called, so the mention is silently dropped — the user gets no task and no feedback. Since thread context is enrichment rather than a hard requirement, it is safer to fall back to the already-enriched `msg.Body` and continue routing. ```go if innerEvent.ThreadTimeStamp != "" { body, err := FetchThreadContext(ctx, h.api, innerEvent.Channel, innerEvent.ThreadTimeStamp, h.botUserID) if err != nil { h.log.Error(err, "Failed to fetch thread context, falling back to message text", "channel", innerEvent.Channel, "threadTS", innerEvent.ThreadTimeStamp) // fall through: msg.Body already contains the raw message text } else { msg.Body = body msg.HasThreadContext = true } } h.routeMessage(ctx, msg) ``` How can I resolve this? If you propose a fix, please make it concise.
-
internal/slack/handler.go, line 1541-1551 (link)taskNameis formatted as<spawnerName>-slack-<12-char hash>. Whenspawner.Nameexceeds 44 characters the combined string exceeds 63 characters and is truncated, chopping off part (or all) of theshortHash. Two different Slack messages from the same spawner then produce the sametaskName; the second message is silently discarded via theIsAlreadyExistspath.Use the hash to derive the task name rather than truncating, e.g.:
combined := fmt.Sprintf("%s-slack-%s", spawner.Name, shortHash) if len(combined) > 63 { // Re-hash the full combined string to a fixed-width name that is always unique fullSum := sha256.Sum256([]byte(combined)) taskName = "slack-" + hex.EncodeToString(fullSum[:])[:12] } else { taskName = combined }
Prompt To Fix With AI
This is a comment left during a code review. Path: internal/slack/handler.go Line: 1541-1551 Comment: **Task name collision for spawner names longer than 44 characters** `taskName` is formatted as `<spawnerName>-slack-<12-char hash>`. When `spawner.Name` exceeds 44 characters the combined string exceeds 63 characters and is truncated, chopping off part (or all) of the `shortHash`. Two different Slack messages from the same spawner then produce the same `taskName`; the second message is silently discarded via the `IsAlreadyExists` path. Use the hash to derive the task name rather than truncating, e.g.: ```go combined := fmt.Sprintf("%s-slack-%s", spawner.Name, shortHash) if len(combined) > 63 { // Re-hash the full combined string to a fixed-width name that is always unique fullSum := sha256.Sum256([]byte(combined)) taskName = "slack-" + hex.EncodeToString(fullSum[:])[:12] } else { taskName = combined } ``` How can I resolve this? If you propose a fix, please make it concise.
-
internal/slack/thread.go, line 1883-1890 (link)BotParticipatedis exported but never calledBotParticipatedis defined and exported but is not referenced anywhere in this PR's handler code. If it is meant to gate thread-reply processing (preventing the bot from responding to threads it never joined), it should be wired intohandleEventsAPI. Otherwise it is dead code and should be unexported or removed to keep the package surface minimal.Prompt To Fix With AI
This is a comment left during a code review. Path: internal/slack/thread.go Line: 1883-1890 Comment: **`BotParticipated` is exported but never called** `BotParticipated` is defined and exported but is not referenced anywhere in this PR's handler code. If it is meant to gate thread-reply processing (preventing the bot from responding to threads it never joined), it should be wired into `handleEventsAPI`. Otherwise it is dead code and should be unexported or removed to keep the package surface minimal. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: internal/slack/handler.go
Line: 1424-1432
Comment:
**Thread reply silently dropped on transient fetch failure**
If `FetchThreadContext` returns an error (e.g. Slack API rate-limit, transient network error), `return` is hit before `h.routeMessage` is called, so the mention is silently dropped — the user gets no task and no feedback. Since thread context is enrichment rather than a hard requirement, it is safer to fall back to the already-enriched `msg.Body` and continue routing.
```go
if innerEvent.ThreadTimeStamp != "" {
body, err := FetchThreadContext(ctx, h.api, innerEvent.Channel, innerEvent.ThreadTimeStamp, h.botUserID)
if err != nil {
h.log.Error(err, "Failed to fetch thread context, falling back to message text",
"channel", innerEvent.Channel, "threadTS", innerEvent.ThreadTimeStamp)
// fall through: msg.Body already contains the raw message text
} else {
msg.Body = body
msg.HasThreadContext = true
}
}
h.routeMessage(ctx, msg)
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: internal/slack/handler.go
Line: 1541-1551
Comment:
**Task name collision for spawner names longer than 44 characters**
`taskName` is formatted as `<spawnerName>-slack-<12-char hash>`. When `spawner.Name` exceeds 44 characters the combined string exceeds 63 characters and is truncated, chopping off part (or all) of the `shortHash`. Two different Slack messages from the same spawner then produce the same `taskName`; the second message is silently discarded via the `IsAlreadyExists` path.
Use the hash to derive the task name rather than truncating, e.g.:
```go
combined := fmt.Sprintf("%s-slack-%s", spawner.Name, shortHash)
if len(combined) > 63 {
// Re-hash the full combined string to a fixed-width name that is always unique
fullSum := sha256.Sum256([]byte(combined))
taskName = "slack-" + hex.EncodeToString(fullSum[:])[:12]
} else {
taskName = combined
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: internal/slack/thread.go
Line: 1883-1890
Comment:
**`BotParticipated` is exported but never called**
`BotParticipated` is defined and exported but is not referenced anywhere in this PR's handler code. If it is meant to gate thread-reply processing (preventing the bot from responding to threads it never joined), it should be wired into `handleEventsAPI`. Otherwise it is dead code and should be unexported or removed to keep the package surface minimal.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(api): Address PR review feedback for..." | Re-trigger Greptile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds
slackas a new source type in the TaskSpawnerWhenstruct, enablingtask creation from Slack messages. This is the minimum viable implementation
for API review — it includes the core triggering flow but defers status
reporting (posting updates back to Slack threads) to a follow-up PR.
What's included:
SlackAPI type with 2 fields:channelsandmentionUserIDskelos-slack-serverbinary that connects to Slack via Socket Mode(outbound WebSocket — no ingress required) and routes messages to matching
TaskSpawners
Deployment created)
What's NOT included:
This MVP does not post any status updates back to Slack — no "accepted",
"done", or "failed" messages appear in the thread. If barebones reporting
(plain text phase transitions) is desired in this PR, that's a small addition
(~170 lines). Otherwise, full rich reporting (Block Kit, live progress,
agent response rendering) will come in a follow-up PR.
Additional filtering coming in follow-up PRs (already implemented in datagravity-ai#75):
triggerCommand— slash command or message prefix (e.g., "/kelos", "!fix") that triggers task creation and is stripped from the promptallowedUsers— restrict which Slack user IDs can trigger tasksexcludeCommands— negative routing to prevent a spawner from firing on messages intended for another spawnerWhich issue(s) this PR is related to:
Towards #932
Special notes for your reviewer:
This is carved out of a larger implementation (datagravity-ai#75) to keep the
API review focused.
Does this PR introduce a user-facing change?
Summary by cubic
Adds Slack as a TaskSpawner source via a centralized
kelos-slack-serverthat uses Socket Mode to spawn tasks from Slack messages. Supports channel filtering, implicit @bot mention, regex triggers, and slash commands; preserves attachments in the task body and uses the first line of text as the task title; posting status back to Slack will come later.New Features
spec.when.slackwithchannelsandtriggers(RE2 patterns, OR semantics, per-triggermentionOptional; emptypatternallowed). CRD validates channel IDs and triggerpatternmax length.kelos-slack-server: leader-elected Socket Mode listener (no ingress), Helm Deployment/ServiceAccount/RBAC; least-privilege RBAC to createtasksonly; usesgithub.com/slack-go/slack.mentionOptional; slash commands bypass mention/trigger; thread replies include fetched context; top-level messages include attachments; titles use the first line of text.Migration
SLACK_BOT_TOKENandSLACK_APP_TOKEN; set.Values.slackServer.secretName, enable with.Values.slackServer.enabled=true, and configure the image if needed.when.slackto TaskSpawners. Use{}to fire on any bot mention, or setchannelsandtriggersto narrow matching (an empty triggerpatternmatches all messages).Written for commit edd1133. Summary will update on new commits. Review in cubic