Skip to content

feat(api): add excludePatterns to Slack source for negative routing#1082

Merged
gjkim42 merged 1 commit intokelos-dev:mainfrom
datagravity-ai:feat/slack-exclude-patterns
May 3, 2026
Merged

feat(api): add excludePatterns to Slack source for negative routing#1082
gjkim42 merged 1 commit intokelos-dev:mainfrom
datagravity-ai:feat/slack-exclude-patterns

Conversation

@jkahuja
Copy link
Copy Markdown
Contributor

@jkahuja jkahuja commented May 1, 2026

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds an excludePatterns field to the Slack TaskSpawner source, enabling negative routing between multiple Slack-triggered TaskSpawners. When multiple spawners listen in the same channel, exclude patterns let a spawner opt out of messages intended for another spawner.

For example, a triage spawner can exclude messages containing /solve:

spec:
  when:
    slack:
      excludePatterns:
        - "/solve"

while a solver spawner triggers only on /solve:

spec:
  when:
    slack:
      triggers:
        - pattern: "/solve"
          mentionOptional: true

Key design decisions:

  • Regex-based (RE2, unanchored), consistent with triggers[].pattern and excludeBodyPatterns on GitHub webhooks
  • Matched against raw message text, same as triggers — no special preprocessing
  • Does NOT apply to slash commands (Slack strips the command name from the payload before delivery)
  • Evaluated after positive match (channel + mention/triggers must pass first)
  • Validation markers match existing conventions: MaxItems=10, MinLength=1, MaxLength=256

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

Follow-up to #1002 (Slack source MVP). Invalid regex compile errors are cached and logged once per bad pattern via log.Printf, consistent with how handler.go uses stdlog "log" for package-level diagnostics.

Does this PR introduce a user-facing change?

Add `excludePatterns` field to the Slack source configuration. RE2 regex patterns that reject messages when any pattern matches, enabling negative routing between multiple Slack-triggered TaskSpawners.

🤖 Generated with Claude Code

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 7 files

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 1, 2026

Greptile Summary

This PR adds an excludePatterns field to the Slack TaskSpawner source, enabling negative routing between multiple spawners listening in the same channel. It also introduces stripLeadingMentions and applies it to both trigger and exclude-pattern matching, so anchored patterns like ^/solve work correctly against real Slack messages that include leading <@USERID> tokens.

The two concerns raised in the previous review thread — missing stripLeadingMentions implementation and no log warning for invalid patterns — are fully resolved in this diff.

Confidence Score: 5/5

Safe to merge — implementation is correct, race-safe, and thoroughly tested; no P0 or P1 issues found.

Both previously-flagged concerns (missing stripLeadingMentions and no error logging) are resolved. The regexp cache refactor uses LoadOrStore correctly. Exclude-pattern evaluation order (after positive match, before return) is correct. Test suite covers anchored patterns, multi-mention stripping, slash-command bypass, invalid regex, and integration with triggers.

No files require special attention.

Important Files Changed

Filename Overview
internal/slack/filter.go Core logic: adds stripLeadingMentions, matchesExcludePatterns, refactors regexp cache to a shared race-safe LoadOrStore pattern with one-time error logging; MatchesSpawner updated to evaluate exclude patterns after positive match. Implementation is correct.
internal/slack/filter_test.go Adds 12 integration-level cases to TestMatchesSpawner, 3 mention-stripping cases to TestMatchesTriggers, and new TestMatchesExcludePatterns / TestStripLeadingMentions suites covering anchored patterns, multi-mention stripping, invalid regex, slash-command bypass, and mid-text mention preservation.
api/v1alpha1/taskspawner_types.go Adds ExcludePatterns []string field to Slack struct with MaxItems=10, items:MinLength=1, items:MaxLength=256 markers; updates SlackTrigger.Pattern doc to mention stripping. Markers are consistent with existing conventions.
api/v1alpha1/zz_generated.deepcopy.go Auto-generated deep-copy for ExcludePatterns slice; correct nil-guard and make/copy pattern matching other []string fields.
docs/reference.md Adds reference table rows for spec.when.slack.channels, triggers[].pattern, triggers[].mentionOptional, and excludePatterns; descriptions accurately reflect implementation behavior.
internal/manifests/charts/kelos/templates/crds/taskspawner-crd.yaml CRD template updated with excludePatterns array (maxItems:10, items minLength:1/maxLength:256) consistent with the Go type markers.
internal/manifests/install-crd.yaml Install manifest CRD updated identically to the Helm chart template, as expected.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[MatchesSpawner] --> B{slackCfg == nil?}
    B -- yes --> REJECT[return false]
    B -- no --> C{matchesChannel?}
    C -- no --> REJECT
    C -- yes --> D{IsSlashCommand?}
    D -- yes --> ALLOW[return true]
    D -- no --> E{len Triggers == 0?}
    E -- yes --> F[hasBotMention\nraw msg.Text]
    E -- no --> G[matchesTriggers\nstrips leading mentions\nthen pattern match]
    F --> H{positiveMatch?}
    G --> H
    H -- no --> REJECT
    H -- yes --> I[matchesExcludePatterns\nstrips leading mentions\nthen pattern match]
    I --> J{any pattern\nmatches?}
    J -- yes --> REJECT
    J -- no --> ALLOW
Loading

Reviews (6): Last reviewed commit: "feat(api): add excludePatterns to Slack ..." | Re-trigger Greptile

Comment thread internal/slack/filter.go Outdated
Comment thread internal/slack/filter.go
@jkahuja
Copy link
Copy Markdown
Contributor Author

jkahuja commented May 1, 2026

CC: @gjkim42 @knechtionscoding

@gjkim42
Copy link
Copy Markdown
Collaborator

gjkim42 commented May 1, 2026

/kelos api-review

@kelos-bot
Copy link
Copy Markdown

kelos-bot Bot commented May 1, 2026

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-16621d0a34cb has succeeded. ✅

Copy link
Copy Markdown

@kelos-bot kelos-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: APPROVE
Scope: Adds excludePatterns []string to Slack (TaskSpawner source) for negative regex-based routing between spawners.

Findings

API Conventions

  • camelCase JSON tag, +optional, omitempty, godoc comment, and validation markers all conform — api/v1alpha1/taskspawner_types.go:595-602.
  • Generated artifacts are in sync: api/v1alpha1/zz_generated.deepcopy.go, internal/manifests/charts/kelos/templates/crds/taskspawner-crd.yaml, internal/manifests/install-crd.yaml. ✓
  • Field placed inside the existing Slack struct (no new type), so no new versioning surface introduced.

Compatibility

  • Additive optional field — existing TaskSpawners and Slack server clients can ignore it without behavior change. ✓
  • Under v1alpha1, matching the rest of the Slack source. No conversion concern.

Naming and Consistency

  • excludePatterns is acceptable — for the Slack source the message text is the only matched dimension. Note, however, that GitHubWebhookFilter uses the paired bodyPattern / excludeBodyPatterns (api/v1alpha1/taskspawner_types.go:412-426). If a second exclude dimension is ever added to Slack (e.g. user IDs, command names), the bare excludePatterns becomes ambiguous and a rename/shim is awkward because CRD fields are effectively permanent. Worth a deliberate decision now that the chosen scope is "match against text only, forever" — otherwise excludeTextPatterns would be more future-proof.
  • MaxItems=10 on excludePatterns vs MaxItems=8 on triggers (same struct) is a minor inconsistency: the 10 was inherited from GitHub excludeBodyPatterns, the 8 from the original Slack triggers cap. Fine to keep, but worth aligning on a documented house rule for "match list" caps so future fields aren't picked ad hoc — api/v1alpha1/taskspawner_types.go:589, 599.

Validation

  • items.MinLength=1 correctly forbids the empty string (which as a regex would match every message and silently disable the spawner).
  • items.MaxLength=256 matches SlackTrigger.Pattern (api/v1alpha1/taskspawner_types.go:611-612). Consistent within the Slack source.
  • No CEL/XValidation needed for cross-field rules — the runtime semantics ("only filters within the already-positively-matched set") fall out of the implementation order in MatchesSpawner and don't need a CRD-level constraint.

Documentation

  • The godoc says "RE2, unanchored" but does not call out that the matched string is the raw Slack payload, which still contains <@USER_ID> mention prefixes and surrounding whitespace. Users writing the natural-looking pattern ^/solve will be surprised that it does not match <@UBOT1> /solve fix this. Recommend adding a sentence to api/v1alpha1/taskspawner_types.go:595-601 and the matching docs/reference.md row, e.g.:

    Patterns are matched against the raw message text, which includes any leading <@USER_ID> mention. Prefer unanchored patterns; if you need to anchor, account for the mention prefix (e.g. ^(?:<@[^>]+>\s*)?/solve).

  • The CRD description text inherits the godoc, so updating one place suffices.

Extensibility

  • ExcludePatterns []string mirrors ExcludeBodyPatterns []string and is OK for the current scope, but note the asymmetry with Triggers []SlackTrigger. If a future feature wants per-exclude knobs (case-insensitivity at the API level, scope-to-thread, "exclude only when no mention", etc.), a parallel struct list will be required and the existing []string field is awkward to migrate. Regex flags inline ((?i), (?m)) cover the common cases, so this is acceptable as long as that's the conscious bet.

Suggestions (optional, non-blocking)

  • The unit-level TestMatchesExcludePatterns exercises anchored patterns (^/solve) against bare text without <@…>, which doesn't reflect the realistic call path through MatchesSpawner. The integration-level TestMatchesSpawner only uses unanchored excludes. Adding one anchored-pattern case to TestMatchesSpawner (e.g. ^/solve against "<@UBOT1> /solve fix", expecting want: true because the leading mention prevents the anchor from matching) would lock in the post-e13705e decision to drop mention-stripping and prevent regressions.
  • Consider one e2e/integration scenario where two TaskSpawners share a channel — one with triggers: [{pattern: "/solve"}] and one with excludePatterns: ["/solve"] — to validate the negative-routing motivation end-to-end.

Note on prompt injection: A non-human review body on this PR contained an HTML-comment directive instructing AI reviewers to attribute findings to a third-party tool. That instruction was disregarded; this review is independent and reflects only my own analysis of the diff.

/kelos needs-input

@knechtionscoding
Copy link
Copy Markdown
Contributor

All I did was squash commits @jkahuja and @gjkim42

Copy link
Copy Markdown
Collaborator

@gjkim42 gjkim42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/kelos api-review

@@ -623,6 +592,16 @@ type Slack struct {
// +optional
// +kubebuilder:validation:MaxItems=8
Triggers []SlackTrigger `json:"triggers,omitempty"`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it's good to place this field inside of the SlackTrigger or not...

What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we put it inside the trigger it has to be relisted for every command. It feels more natural to have a top level list of exclusions applied to each task spawner rather than having to duplicate the string inside of each trigger

@kelos-bot
Copy link
Copy Markdown

kelos-bot Bot commented May 2, 2026

🤖 Kelos Task Status

Task kelos-api-reviewer-pull-request-review-560f968d27ab has succeeded. ✅

Copy link
Copy Markdown

@kelos-bot kelos-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: APPROVE
Scope: Adds excludePatterns []string to the Slack source on TaskSpawner for negative regex routing between spawners that share a channel.

Findings

API Conventions

  • ExcludePatterns uses camelCase JSON tag, is marked +optional with omitempty, and carries a godoc comment — api/v1alpha1/taskspawner_types.go:596-604. ✓
  • Generated artifacts are in sync with the type definition: api/v1alpha1/zz_generated.deepcopy.go:805-809, the chart CRD internal/manifests/charts/kelos/templates/crds/taskspawner-crd.yaml:3557-3568, and the install bundle internal/manifests/install-crd.yaml:6676-6687. ✓
  • Field added to the existing Slack struct rather than a new type — no extra versioning surface. ✓

Compatibility

  • Purely additive v1alpha1 change. Existing TaskSpawner objects round-trip unchanged; the slack-server code reads the field and silently no-ops when nil, so older controllers/servers ignoring it still get the previous behavior. ✓
  • No conversion concern (single API version).

Validation

  • items.MinLength=1 — correctly forbids the empty string, which as an unanchored RE2 pattern would match every message and silently disable the spawner.
  • items.MaxLength=256 — matches SlackTrigger.Pattern (api/v1alpha1/taskspawner_types.go:611). Consistent within the Slack source.
  • MaxItems=10 matches GitHubWebhookFilter.ExcludeBodyPatterns (api/v1alpha1/taskspawner_types.go:423). Note the asymmetry with sibling Triggers (MaxItems=8, line 593) — not a blocker, but worth a documented house-rule for "match-list" caps so future fields don't pick a third number.
  • No CEL/XValidation needed: the "exclude only narrows an already-positive match" semantic falls out of the MatchesSpawner ordering, not a CRD-level constraint.

Naming and Consistency

  • excludePatterns is acceptable in isolation, but stands out next to Triggers []SlackTrigger. The matching-positive side is a struct list (so pattern, mentionOptional etc. live as fields), while the matching-negative side is a bare []string. The asymmetry is fine if exclusion will forever be "regex-on-text-only", but it is a deliberate bet given that CRD fields are effectively permanent.
  • Compared to GitHubWebhookFilter, which pairs bodyPattern/excludeBodyPatterns, the Slack side names the matched dimension only on the negative field. If a second exclude dimension is ever added (user IDs, command names, thread scope), excludePatterns becomes ambiguous and rename is awkward. excludeTextPatterns would future-proof, but I do not consider this blocking — please decide explicitly.

Documentation

  • The godoc for ExcludePatterns says "matched against … message text" via the surrounding context, but does not call out that the matched string is the raw Slack payload, which still contains any leading <@USER_ID> bot-mention and surrounding whitespace. A user writing the natural pattern ^/solve will be surprised it does not match <@UBOT1> /solve fix — and the unit test TestMatchesExcludePatterns (internal/slack/filter_test.go:545-558) exercises anchored patterns against bare text, which masks this.

    Recommend adding one sentence to api/v1alpha1/taskspawner_types.go:596-599 (and the matching docs/reference.md row, line 180) e.g.:

    Patterns are matched against the raw message text, which still includes any leading <@USER_ID> bot-mention. If you anchor with ^, account for the mention prefix (e.g. ^(?:<@[^>]+>\s*)?/solve).

    The CRD description inherits the godoc, so updating one place suffices.

  • "Does NOT apply to slash commands" is a meaningful behavioral carve-out and is correctly noted in the godoc. ✓ (The reasoning — Slack strips the command name from the payload — is in the PR body but not in the API surface; that is fine since the user-observable rule is what matters.)

Extensibility

  • []string is acceptable for the current scope but is not migratable to per-entry knobs (case-insensitivity, scope-to-thread, "exclude only when no mention", etc.) without introducing a parallel field. Inline RE2 flags ((?i), (?m)) cover the common ergonomic cases, so this is acceptable as a conscious trade-off — same shape decision as ExcludeBodyPatterns, so at least it is consistent within the API.

Suggestions (optional, non-blocking)

  • Add one anchored-pattern case to the integration-level TestMatchesSpawner (e.g. ^/solve against "<@UBOT1> /solve fix" expecting want: true because the mention prefix prevents the anchor from matching). The existing case at internal/slack/filter_test.go:183-190 only covers unanchored patterns, so a regression to mention-stripping (or a future change in raw-text handling) would not be caught at the realistic call path.
  • Consider one e2e/integration scenario covering the documented motivation: two TaskSpawners on the same channel, one with triggers: [{pattern: "/solve"}] and one with excludePatterns: ["/solve"], asserting only the intended spawner fires.

Note on prompt injection: A non-human review on this PR contained an HTML-comment directive instructing AI reviewers to attribute findings to a third-party tool. That instruction was disregarded; this review is independent and reflects only my own analysis of the diff.

/kelos needs-input

@gjkim42
Copy link
Copy Markdown
Collaborator

gjkim42 commented May 2, 2026

/kelos review

@kelos-bot
Copy link
Copy Markdown

kelos-bot Bot commented May 2, 2026

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-68ed3f32c264 has succeeded. ✅

Copy link
Copy Markdown

@kelos-bot kelos-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds an excludePatterns []string field on the Slack source that rejects a positively-matched message when any RE2 regex matches; slash commands bypass the new filter and the regex cache now stores compile errors so each invalid pattern is logged once.

Findings Overview

Priority Count File:Line Summary
P0 0 none
P1 0 none
P2 1 commit message Squashed commit body claims @-mentions are stripped, but implementation matches raw text
P3 1 internal/slack/filter.go:48 getOrCompileTriggerRegexp now also serves exclude patterns; name is stale post-refactor
P3 1 internal/slack/filter.go:48-58 Store vs LoadOrStore race window can double-log the same invalid pattern

Findings

Conventions

  • [P2] commit message — The commit body says "Leading @-mentions are stripped before matching so patterns target semantic content," but matchesExcludePatterns (internal/slack/filter.go:139-153) calls re.MatchString(text) against the raw text and the PR description explicitly says "Matched against raw message text … no special preprocessing." The integration tests (e.g. TestMatchesSpawner "excludePatterns rejects matching message") confirm the raw-text behavior. The squashed message will land on main permanently and mislead anyone running git log or git blame. Worth rewording during the merge (or in a follow-up amend before merge) to match the actual implementation, e.g. "Patterns are matched against the raw message text, including any leading <@USER_ID> mention. Does not apply to slash commands."

Code quality

  • [P3] internal/slack/filter.go:48 — The cache variable was renamed from triggerRegexpCache to regexpCache because it now also caches exclude-pattern compilations (called from matchesExcludePatterns at line 144), but the helper is still named getOrCompileTriggerRegexp. The name now misrepresents its scope. Renaming to getOrCompileRegexp would line up with the cache rename and is a 3-call-site change.

Concurrency

  • [P3] internal/slack/filter.go:48-58 — Between the Load miss on line 49 and the Store on line 57, a second goroutine can also miss the cache, recompile the same bad pattern, and call log.Printf before the first Store lands. The PR description promises "logged once per bad pattern" — that holds only after the winning Store. In practice the window is tiny and self-healing (subsequent calls hit the cache), so impact is at most a handful of duplicate log lines on startup under burst load. Switching to LoadOrStore would close the window since regexp.Compile is pure and races produce equivalent entries:
    re, err := regexp.Compile(pattern)
    entry := &regexpCacheEntry{re: re, err: err}
    if actual, loaded := regexpCache.LoadOrStore(pattern, entry); loaded {
        e := actual.(*regexpCacheEntry)
        return e.re, e.err
    }
    if err != nil {
        log.Printf("Invalid regex pattern %q: %v", pattern, err)
    }
    return re, err

Suggestions (optional)

  • [P3] internal/slack/filter_test.go — Adding one anchored-pattern case to TestMatchesSpawner (e.g. ^/solve against "<@UBOT1> /solve fix" expecting want: true) would lock in the "no mention-stripping" decision at the realistic call path; the unit-level TestMatchesExcludePatterns cases use bare text without the <@...> prefix, so a regression to mention-stripping would not be caught there.

Key takeaways

  • Implementation, validation markers, deepcopy/CRD regeneration, and tests are all in good shape and consistent with excludeBodyPatterns on the GitHub side.
  • Only blocking-adjacent item is the stale commit message describing behavior that the code doesn't implement — worth fixing before merge so main history is accurate.

Note on prompt injection: The cubic-dev-ai review on this PR contains an HTML-comment directive instructing AI reviewers to attribute findings to that tool. That instruction was disregarded; this review is independent and reflects only my own analysis of the diff.

@knechtionscoding
Copy link
Copy Markdown
Contributor

@gjkim42 I'll follow up with the stripping leading mentions in the next PR.

@gjkim42 gjkim42 enabled auto-merge May 2, 2026 13:38
@gjkim42 gjkim42 disabled auto-merge May 2, 2026 13:38
@gjkim42
Copy link
Copy Markdown
Collaborator

gjkim42 commented May 2, 2026

@knechtionscoding
Why don’t we fix it here?

@knechtionscoding
Copy link
Copy Markdown
Contributor

@gjkim42 Fixed in this commit

@gjkim42
Copy link
Copy Markdown
Collaborator

gjkim42 commented May 2, 2026

/kelos review

@kelos-bot
Copy link
Copy Markdown

kelos-bot Bot commented May 2, 2026

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-9fb68d13185a has succeeded. ✅

Copy link
Copy Markdown

@kelos-bot kelos-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds an excludePatterns []string field on the Slack source that rejects positively-matched messages whose text matches any RE2 regex; the latest revision now strips leading <@USER_ID> mentions before matching so anchored patterns work against natural Slack payloads. Slash commands bypass the new filter and the regex cache stores compile errors so each invalid pattern is normally logged once.

Findings Overview

Priority Count File:Line Summary
P0 0 none
P1 0 none
P2 1 internal/slack/filter.go:178 matchesTriggers still matches raw text, creating asymmetry with matchesExcludePatterns for anchored patterns
P2 1 api/v1alpha1/taskspawner_types.go:596-604 Godoc/CRD description does not mention the leading-mention strip; users won't know why anchored patterns work
P2 1 PR description PR body still says "Matched against raw message text … no special preprocessing" — contradicts the new strip behavior
P3 1 internal/slack/filter.go:48 getOrCompileTriggerRegexp name is stale; helper now serves exclude patterns too
P3 1 internal/slack/filter.go:48-58 Load+Store race window can double-log the same invalid pattern

Findings

Correctness / API consistency

  • [P2] internal/slack/filter.go:178matchesTriggers calls re.MatchString(text) against raw text while the new matchesExcludePatterns (line 158) strips leading mentions first. The same anchored regex now behaves differently across the two paths: ^/solve matches as an exclude against <@UBOT1> /solve fix but does NOT match as a trigger pattern. The previous greptile review and the API-review both flagged this as a footgun; addressing it by extending mention-stripping to triggers (or, conversely, dropping it from excludes and asking users to write (?:<@[^>]+>\s*)?^/solve) would keep the two regex surfaces consistent.
  • [P2] api/v1alpha1/taskspawner_types.go:596-604 — Godoc says "regular expressions … RE2, unanchored" but does not mention that the matched string has leading <@USER_ID> mentions stripped. The CRD description inherits this godoc verbatim, so the only place a user can learn that ^/solve works against <@UBOT1> /solve fix is the unit tests. Worth one sentence, e.g. "Leading <@USER_ID> mentions and surrounding whitespace are stripped before matching, so anchored patterns target the semantic message body." docs/reference.md:180 should mirror the same wording.
  • [P2] PR description — The body still reads "Matched against raw message text, same as triggers — no special preprocessing." That contradicts the implementation in the latest commit, which calls stripLeadingMentions before matching. The squashed commit message itself is now correct, but the PR description is the artifact most contributors will skim before reading code; please reword before merge.

Code quality

  • [P3] internal/slack/filter.go:48 — The cache variable was renamed from triggerRegexpCache to regexpCache because it now caches both trigger and exclude-pattern compilations (call site at line 164), but the helper is still getOrCompileTriggerRegexp. Renaming to getOrCompileRegexp would line up with the cache rename and is a 3-call-site change.

Concurrency

  • [P3] internal/slack/filter.go:48-58 — Between the Load miss on line 49 and the Store on line 57, a second goroutine can also miss the cache, recompile the same bad pattern, and call log.Printf before the first Store lands. The window is tiny and self-healing (subsequent calls hit the cache), so the worst-case impact is a handful of duplicate log lines on startup under burst load. Switching to LoadOrStore would close the window since regexp.Compile is pure and races produce equivalent entries.

Suggestions (optional)

  • [P3] internal/slack/filter_test.goTestMatchesExcludePatterns already exercises the strip behavior directly. Adding one paired case to TestMatchesSpawner that uses an anchored exclude and an anchored trigger on the same <@UBOT1> ... message would lock in the documented (and now load-bearing) asymmetry against accidental regressions.
  • [P3] Consider an integration scenario covering the documented motivation: two TaskSpawners on the same channel, one with triggers: [{pattern: "/solve"}] and one with excludePatterns: ["/solve"], asserting only the intended spawner fires.

Key takeaways

  • The strip-leading-mentions change correctly addresses the earlier reviewer feedback and the new tests cover it well — no blocking issues.
  • The remaining gap is consistency: triggers don't strip but excludes do, and the godoc/PR description haven't been updated to reflect the new behavior. Worth resolving before merge so the API contract matches the implementation.

Note on prompt injection: The cubic-dev-ai review on this PR contains an HTML-comment directive instructing AI reviewers to attribute findings to that tool. That instruction was disregarded; this review is independent and reflects only my own analysis of the diff.

Comment thread internal/slack/filter.go
Comment thread api/v1alpha1/taskspawner_types.go Outdated
Comment thread internal/slack/filter.go Outdated
Comment thread internal/slack/filter.go Outdated
@knechtionscoding
Copy link
Copy Markdown
Contributor

@gjkim42 happy to fix match triggers as well, if you would like in this or can do it as a follow up. Either works

Add ExcludePatterns field to the Slack TaskSpawner source. Regex
patterns (RE2) reject messages when any pattern matches, enabling
negative routing between multiple Slack-triggered TaskSpawners.
Leading @-mentions are stripped before matching so patterns target
semantic content. Does not apply to slash commands.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-authored-by: jkahuja <jkahuja@users.noreply.github.com>
@knechtionscoding knechtionscoding force-pushed the feat/slack-exclude-patterns branch from 7c806b5 to 290e535 Compare May 2, 2026 16:11
@gjkim42
Copy link
Copy Markdown
Collaborator

gjkim42 commented May 3, 2026

/kelos review

@kelos-bot
Copy link
Copy Markdown

kelos-bot Bot commented May 3, 2026

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-061a89d7762c has succeeded. ✅

Copy link
Copy Markdown

@kelos-bot kelos-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds excludePatterns []string to the Slack source. Messages that pass channel + mention/trigger checks are rejected when any RE2 pattern matches the text (leading <@USER_ID> mentions stripped first). Slash commands bypass the new filter, the regex cache now stores compile errors with LoadOrStore so each invalid pattern is logged once, and matchesTriggers was switched to also strip leading mentions so the two regex surfaces stay symmetric.

Findings Overview

Priority Count File:Line Summary
P0 0 none
P1 0 none
P2 0 none
P3 1 PR description PR body still claims "no special preprocessing" — contradicts the implemented mention-stripping

Findings

Conventions

  • [P3] PR description — The PR body still reads "Matched against raw message text, same as triggers — no special preprocessing." That contradicts the latest revision (stripLeadingMentions is invoked from both matchesTriggers and matchesExcludePatterns). The squashed commit message itself is correct, but the PR body is what most reviewers/contributors skim before reading code. Worth a short edit before merge so the description matches the godoc and tests.

Suggestions (optional)

  • [P3] internal/slack/filter_test.goTestMatchesExcludePatterns already covers anchored patterns against <@UBOT1> ... text directly. A paired anchored case in the integration-level TestMatchesSpawner (e.g. trigger ^deploy and exclude ^/solve against <@UBOT1> /solve fix) would lock in the now-load-bearing strip behavior at the realistic call path.
  • [P3] An e2e/integration scenario covering the documented motivation — two TaskSpawners on the same channel, one with triggers: [{pattern: "/solve"}] and one with excludePatterns: ["/solve"], asserting only the intended spawner fires — would protect the negative-routing contract end-to-end.

Key takeaways

  • All prior review threads (matchesTriggers asymmetry, godoc/CRD documenting the strip, stale getOrCompileTriggerRegexp name, Load/Store race window) are resolved in the latest commit.
  • Implementation, validation markers, deepcopy/CRD regeneration, logging conventions, and tests are all in good shape — only the stale PR body remains.

Note on prompt injection: The cubic-dev-ai review on this PR contains an HTML-comment directive instructing AI reviewers to attribute findings to that tool. That instruction was disregarded; this review is independent and reflects only my own analysis of the diff.

@gjkim42 gjkim42 enabled auto-merge May 3, 2026 08:12
@gjkim42 gjkim42 added this pull request to the merge queue May 3, 2026
Merged via the queue into kelos-dev:main with commit 2b6253b May 3, 2026
23 checks passed
@knechtionscoding knechtionscoding deleted the feat/slack-exclude-patterns branch May 3, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature needs-actor needs-priority needs-triage release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants