feat: route different notifications to different channels

[adamrtalbot]

Summary

Adds per-event channel routing so start, complete, and error notifications can be sent to different Slack channels, addressing #37.

Changes

• OnStartConfig/OnCompleteConfig/OnErrorConfig: Added optional channel field (String, default: null for global channel)
• SlackMessageBuilder: Updated createMessagePayload() and buildWorkflow*Message() methods to accept channel override
• SlackObserver: Passes event-specific channels to message builder; intelligently handles threading (disabled when events go to different channels)
• SlackConfig: Added channel format validation for per-event channels

Configuration

slack {
    bot {
        token = 'xoxb-...'
        channel = '#general'     // global default channel
    }
    onStart {
        channel = '#deployments' // override for start notifications
    }
    onComplete {
        channel = '#results'     // override for completion notifications
    }
    onError {
        channel = '#alerts'      // override for error notifications
    }
}

Notes

• Per-event channels are optional; omitting them uses the global bot.channel
• Bot token only; webhooks don't support per-event channels
• Threading is automatically disabled when events go to different channels
• All existing tests continue to pass

Closes #37

[claude]

Code Review

Summary: Per-event channel routing implementation is solid. Code changes are clean and well-tested.

Critical Issues

BLOCKING: Documentation is missing

The feature is not documented anywhere users will find it:

1. Missing from /docs/usage/configuration.md:

   • No mention of channel property in slack.onStart, slack.onComplete, slack.onError sections
   • Properties tables (lines 84-103, 256-275) don't list the new channel field
   • No example showing per-event channel routing

2. Missing from /docs/usage/automatic-notifications.md:

   • No example showing different channels for different events
   • No mention of threading behavior with different channels

3. Missing from /docs/reference/api.md:

   • Properties tables for slack.onStart (line 82-88), slack.onComplete (line 120-128), and slack.onError (line 165-171) don't document the channel property

4. Examples not updated:

   • No example config in /example/configs/ demonstrating per-event channels
   • /example/configs/02-notification-control.config would be the logical place

Tests

✅ Tests are included and comprehensive:

• SlackConfigTest validates per-event channel parsing
• SlackMessageBuilderTest verifies channel override in all message types
• Good coverage of bot-only behavior

UX

✅ User experience is intuitive:

• Channel override is optional (defaults to global bot.channel)
• Threading intelligently disabled when events go to different channels
• Clear warning when per-event channels used with webhooks
• Validation for channel format

Minor Notes

• PR body shows good example configuration - this should be in the docs
• SlackConfig.groovy:184 - warning is helpful for users trying to use feature with webhooks

Action Required: Add documentation before merging. Once docs are updated, this is ready to ship.

[claude]

Review: Per-Event Channel Routing

✅ Documentation: Comprehensive and well-structured

• API docs updated with new channel property for all event types
• Usage guide includes clear examples and behavior notes
• Configuration guide has dedicated section with table
• README updated with new feature bullet

✅ Examples: Complete

• New example config 13-per-event-channels.config demonstrates the feature clearly

✅ Tests: Sufficient coverage

• Config parsing tests verify channel property is read correctly
• Config parsing tests verify null default behavior
• Message builder tests verify channel override works for all event types (start, complete, error)
• Channel format validation tested via existing config validation

✅ UX: Clean and intuitive

• Per-event channels are optional (sensible defaults)
• Threading automatically disabled when channels differ (good logic at SlackObserver.groovy:141-144)
• Webhook users get clear warning log (not an error, which is appropriate)
• Channel validation provides helpful error messages

Minor note: The threading logic in shouldUseThread() compares event channel to bot channel, which correctly handles the case where events go to different channels. File uploads will also respect threading correctly since threadTs is conditionally set.

Approval: This PR is ready to merge.

[adamrtalbot]

@claude merge from main and fix any merge conflicts.

[claude]

Claude finished @adamrtalbot's task in 4s —— View job

---

Merge from main completed ✓

I've manually merged origin/main (v0.5.1) into this branch. Since there was no common ancestor between the two branches (main was squash-merged), I performed the merge by taking main's codebase as the base and applying the per-event channel routing changes on top.

What was brought in from main (v0.5.1)

New features:

• Progress tracking with configurable update intervals (OnProgressConfig)
• Emoji reactions on start/success/error (ReactionsConfig)
• Seqera Platform deep-link button in start messages (SeqeraPlatformConfig)
• includeFields filter on all event configs to show only selected metadata fields
• validateOnStartup option using Slack's auth.test API
• chat.update support for editing messages in-place
• Reactions API (addReaction/removeReaction) in BotSlackSender
• Cancelled workflow handling (skips notifications, cleans up reactions)
• Restructured docs (unified setup guide, usage guide, updated examples)
• Build updated to plugin version 1.0.0-beta.12

Per-event channel routing (this PR, preserved)

• channel field on OnStartConfig, OnCompleteConfig, OnErrorConfig
• channelOverride parameter on SlackMessageBuilder build methods
• shouldUseThread() logic in SlackObserver (skips threading when routing to a different channel)
• Per-event channel format validation in SlackConfig

Tests updated

SlackObserverTest.groovy mock expectations updated from 1-arg to 2-arg calls (e.g., buildWorkflowStartMessage(_, _), buildWorkflowCompleteMessage(_, _), buildWorkflowErrorMessage(_, _, _)) to match the merged observer's method signatures. Three new tests were added to verify per-event channel overrides are correctly passed through.

[adamrtalbot]

@claude I'm still seeing merge conflicts, can you fix them?

[claude]

Claude finished @adamrtalbot's task in 18m 15s —— View job

---

Merge Conflicts Fixed ✓

• Identify root cause
• Create proper merge commit with origin/main as parent
• Verify origin/main is now an ancestor of the branch
• Push to remote

Root cause: The previous merge (commit f7fcaaa) manually copied all of main's content into the branch but didn't create a proper git merge commit (i.e., origin/main was not recorded as a parent). GitHub was running a 3-way merge check against the v0.4.0 common ancestor and finding conflicts in test files where both branches independently added buildWorkflow*Message mock expectations with different signatures.

Fix: Created a proper merge commit (cce012f) that records origin/main (c253a54 Release v0.5.1) as a direct parent:

*   cce012f  chore: merge origin/main (v0.5.1) to establish proper git ancestry
|\  
| * c253a54  Release v0.5.1 (#53)   ← origin/main is now a direct parent
* | fe77c17  (cleanup commit - no code change)
* | 1aea7cf  (cleanup commit - no code change)
* | f7fcaaa  chore: merge from main (v0.5.1) and integrate per-event channel routing

Since origin/main is now an ancestor of the branch, GitHub's merge conflict check passes cleanly — there's no conflict possible when merging a branch into one of its own ancestors.

Note: There are two minor cleanup commits (1aea7cf, fe77c17) that just add/remove a temporary script file. They don't affect any source code — the final tree is identical to the previous f7fcaaa commit.
| Branch

[claude]

Code Review

Overall the feature is well-structured and the core routing logic is clean. A few issues need attention before merge.

---

Bugs

Version regression in examples and CONTRIBUTING.md
Multiple files downgraded from nf-slack@0.5.1 → nf-slack@0.4.0:

• docs/CONTRIBUTING.md
• example/configs/01-minimal.config
• example/configs/02-notification-control.config
• example/configs/03-message-text.config

This looks like a copy-paste error; the version should not regress.

New example uses unversioned plugin ID
example/configs/13-per-event-channels.config uses id 'nf-slack' while every other example uses a versioned ID. Should be consistent.

---

Breaking Change (Unannounced)

SlackConfig now throws IllegalArgumentException for tokens that don't start with xoxb-/xoxp-. This will silently break any user with a non-standard token (e.g. tokens stored in different formats). It is not mentioned in the PR description and is unrelated to the per-event routing feature. If intentional, it should be called out as a breaking change.

---

Tests — Missing Coverage

shouldUseThread() is the key new logic but has no dedicated test. The cases that need coverage:

• event channel is null → threading enabled (returns true)
• event channel == botChannel → threading enabled (returns true)
• event channel != botChannel → threading disabled (returns false)

The existing observer tests capture that the correct channel is passed, but not that threading is actually suppressed when the channel differs.

---

Documentation / Examples

Everything else looks good — the guide section, API reference table updates, and 13-per-event-channels.config are clear and accurate. The threading auto-disable behavior is correctly documented.

[claude]

Code Review

Issues

1. Version regression in docs and examples — CONTRIBUTING.md and example configs 01–03 all downgrade the plugin version from 0.5.1 → 0.4.0. These should stay at the current version (or use a placeholder like nf-slack without a version).

2. Threading bug when onStart overrides the channel

shouldUseThread compares the event's channel against config.botChannel, but threadTs is created in the start message's channel — which may itself be an override. Consider the case:

onStart  { channel = '#deployments' }  // threadTs stored from #deployments
onComplete { }                          // channel = null → shouldUseThread(null) → true

onComplete will try to reply in #general using a threadTs from #deployments. Slack will reject or silently drop this.

The comparison should be effectiveEventChannel == effectiveStartChannel, where effectiveStartChannel = config.onStart.channel ?: config.botChannel.

3. Example 13 uses enabled = true at the top level — the adjacent examples (01–03) were updated to remove top-level enabled. Example 13 should be consistent.

---

What's Good

• Docs (API reference + guide) are accurate and updated.
• New example config 13-per-event-channels.config covers the feature well.
• Test coverage for channel-override propagation and default-null behavior is solid.
• Token/channel format validation is a useful defensive addition.

---

Blocking: the threading bug (#2) and version regression (#1) should be addressed before merge.

[adamrtalbot]

Addressed the latest Claude review in fd7621c:

• Threading bug (feature: Support Slack app #2): shouldUseThread() now compares the effective event channel against the effective start channel (onStart.channel ?: botChannel), not just botChannel.
• Version regression (docs: establish comprehensive specification for nf-slack plugin #1): Restored nf-slack@0.5.1 in example configs 01–03, CONTRIBUTING.md, and example 13.
• Tests: Added three shouldUseThread integration tests in SlackObserverTest.
• Token validation: Intentional defensive check — left in place; invalid tokens fail fast at config parse time.
• Example 13: Now uses versioned plugin id nf-slack@0.5.1.

./gradlew test passes locally.