feat: resolve Slack @mentions by display name

[adamrtalbot]

Summary

• Add SlackMentionResolver to translate <@DisplayName> and <!subteam^TeamName> into Slack user/subteam IDs before bot messages are sent
• Match users by username, display name, then real name (case-insensitive); warn and skip ambiguous or missing matches
• Cache users.list / usergroups.list for the pipeline run; warn when webhooks cannot resolve display-name mentions

Test plan

• ./gradlew test (168 tests passing)
• Unit tests for user/subteam resolution, ambiguity, JSON payloads, caching, and webhook warning

Closes #46

Made with Cursor

[claude]

Code Review

Overall: Solid feature addition with good documentation and test coverage. A few issues worth addressing.

Tests

Weak webhook test (SlackClientTest.groovy): The test 'should warn webhook users about unresolved display-name mentions' only asserts noExceptionThrown() — it doesn't verify the warning was actually logged. Since the real HTTP request fails silently (exception caught), this test always passes regardless of whether the warning logic works. Either verify the log output with a spy/mock or at least assert the behavior directly.

Missing test: No test for fetchUsersPage returning a 200 response with ok: false — this path (line ~297: return response.ok ? response : null) returns null silently. The failure path of fetchAllUsers when users is empty would also be good to test.

Thread Safety

cachedUsers and cachedUsergroups are not volatile. In a concurrent scenario, two threads could both observe null and trigger duplicate API fetches. Given this is called from sendMessage/updateMessage, concurrent pipelines could hit this. The result is benign (same data, just a wasted request), but a simple volatile or synchronized block on ensureUsersLoaded() would make the intent clear.

Documentation

• Setup docs correctly list the new scopes with links. ✅
• Usage guide example is clear and covers both user and subteam mention formats. ✅
• The webhook-cannot-resolve warning behavior is documented. ✅

UX

The fallback behavior (leave mention unresolved + warn) is the right call — silent corruption would be worse. The priority order (username → display name → real name) is sensible.

[claude]

Code Review

Overall: well-structured feature with solid test coverage and clear docs.

Tests

The SlackMentionResolverTest is thorough — covers ambiguity, caching, JSON traversal, deleted/bot user filtering, and passthrough for already-resolved IDs.

Weak assertion in webhook test (SlackClientTest): the test only verifies noExceptionThrown() but doesn't assert the warning was actually logged. Since sendMessage makes a real HTTP call to hooks.slack.com which will fail in CI, this test passes vacuously. Consider spying on the logger or checking the loggedWarnings set.

Thread safety

cachedUsers and cachedUsergroups are plain fields (not volatile). If BotSlackSender is called concurrently (e.g. from multiple pipeline event hooks), fetchAllUsers() could be invoked more than once. Not a data-corruption risk — just a redundant API call — but worth noting.

Documentation

Docs in setup.md and guide.md are accurate and well-placed. The bot-only note for webhook users is clearly communicated.

Minor

resolveInJson round-trips through JsonBuilder, which re-serializes the payload (whitespace changes). Fine for the Slack API semantics, but the original formatting is lost.

[adamrtalbot]

Suggested change

	* Copyright 2025, Seqera Labs
	* Copyright 2026, Seqera Labs

[adamrtalbot]

Updated to 2025-2026 to match the rest of the plugin (not 2026 alone).

[adamrtalbot]

What if a users name starts with U or W?

[adamrtalbot]

Good catch. Resolved mentions are now detected by Slack’s uppercase ID shape (U/W + 4+ alphanumerics, optional |label), so mixed-case display names like <@Ulysses> still resolve. Added a test for that case.

[adamrtalbot]

Suggested change

	* Copyright 2025, Seqera Labs
	* Copyright 2026, Seqera Labs

[adamrtalbot]

Same — 2025-2026 here too.

[adamrtalbot]

Addressed Claude review in 27f79ff:

• Thread safety: Double-checked locking for cachedUsers / cachedUsergroups.
• Webhook test: Now spies on warnIfUnresolvedMentions() instead of only asserting no exception.
• fetchUsersPage ok:false: New test verifies graceful passthrough.
• U/W name prefix (inline comment): The regex negative lookahead only skips Slack user/workspace IDs (U/W + alphanumeric), not display names like Uma — those are resolved normally.

./gradlew test passes locally.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> and <!subteam^TeamName> mentions into Slack user/subteam IDs before sending, with caching, ambiguity warnings, and a webhook warning. The docs and test suite are solid overall. A few correctness issues need attention before merge.

---

Bugs

1. Empty cache permanently set on API failure
SlackMentionResolver.groovy — ensureUsersLoaded() / ensureUsergroupsLoaded()

When fetchAllUsers() fails on the first page it returns [] (not null), which gets assigned to cachedUsers. On the next call, cachedUsers != null is true, so the cache is considered warm and the API is never retried. All subsequent <@DisplayName> mentions silently pass through unresolved for the pipeline run's lifetime.

Fix: only assign cachedUsers when the fetch succeeds; on failure, leave it null so the next call retries.

2. <@DisplayName|label> pipe-label format breaks resolution
SlackMentionResolver.groovy — USER_MENTION_PATTERN (line ~41)

The capture group ([^>]+) includes any |label suffix, so <@Jane|jane.doe> queries for "Jane|jane.doe" — no match, spurious warning. The subteam pattern already handles this with ([^>|]+)(?:\|[^>]+)?. Apply the same fix to the user pattern.

3. Field-priority loop bails out early on first ambiguous field
SlackMentionResolver.groovy — resolveUserId() lines ~163–170

If name matches multiple users, the method immediately warns and returns null, skipping display_name and real_name. A unique display-name match can exist even when name is ambiguous. The ambiguity bail-out should only fire after all three fields are exhausted without a unique hit.

4. Mid-pagination failure is silent
SlackMentionResolver.groovy — fetchAllUsers() lines ~280–300

If page 1 succeeds and a subsequent page returns null, the loop breaks and caches the partial list silently (users.isEmpty() is false, suppressing the warning). Users on pages 2+ will never be resolvable. Add a warning when pagination is interrupted.

5. uploadFile comment bypasses mention resolution
BotSlackSender.groovy

sendMessage and updateMessage now call mentionResolver.resolveInJson(), but uploadFile's options.comment string goes directly to the Slack API without resolution. options.comment = "Done <@Jane>" will post the raw unresolved mention.

---

Tests

6. No test verifies BotSlackSender actually calls resolveInJson

Test helpers override postToSlack/postUpdate but mentionResolver is a concrete field — removing the resolveInJson call from sendMessage or updateMessage would not fail any test. Consider adding an assertion via Spy(SlackMentionResolver) or a protected override.

---

Docs

• The guide doesn't mention that user/group lists are cached for the entire pipeline run. Worth one sentence so users know that workspace membership changes mid-run won't be picked up.
• No test covers the multi-page pagination path (large workspaces with >200 members).

[adamrtalbot]

Thanks for the feedback — pushed a simplification pass in 6c61029.

Removed / simplified

• Dropped the synchronized per-run caching layer (volatile fields, double-checked locking, warnOnce dedup set). User/group lists are still loaded lazily on first mention (one users.list pagination + one usergroups.list per BotSlackSender lifetime), which is enough for an ephemeral pipeline run.
• Consolidated HTTP calls into a single apiGet() helper instead of separate fetchUsersPage / fetchUsergroups connection boilerplate.
• Removed the caching-specific unit test; kept coverage for resolution, ambiguity, JSON payloads, webhook warning, and API failure.

Unchanged behavior (issue #46)

• <@DisplayName> → <@U…> via username → display name → real name matching
• <!subteam^TeamName> → <!subteam^S…> via name/handle
• Ambiguous matches warn and stay unresolved
• Webhook mode warns that display-name mentions need a bot token

Copyright

• Updated all source headers on this branch to Copyright 2025-2026.

Size

• SlackMentionResolver: 360 → 293 lines (−67)
• SlackMentionResolverTest: 249 → 214 lines (−35)
• Overall PR vs main: ~665 → ~584 net lines

./gradlew test — 167 tests passing.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> and <!subteam^TeamName> patterns into Slack user/subteam IDs before sending, with caching, pagination, and a webhook-mode warning. Documentation and examples are included and accurate.

---

Bugs

1. Data race on lazy-init cache — SlackMentionResolver.groovy lines 219–229

users and usergroups are plain (non-volatile) instance fields with no synchronization on the null-check-and-set path. SlackObserver drives both sendMessage and updateMessage from different threads (a timer thread fires progress updates; the main observer thread fires onComplete/onError). Two concurrent callers can both observe null and both call fetchAllUsers() — the write on one thread is not guaranteed visible on the other under the JMM. Fix: declare both fields volatile, or guard with synchronized(this).

2. Silent truncation on mid-pagination failure — SlackMentionResolver.groovy line 441

fetchAllUsers only logs a warning when result.isEmpty(). If pages 1–N succeed but page N+1 fails (rate-limit, network blip), the partial list is silently returned and cached as authoritative. Users on subsequent pages will never resolve, with no log message. Fix: warn on any page failure, not just the first.

3. Early-return on ambiguity skips potentially unique later fields — SlackMentionResolver.groovy line 154

resolveUserId iterates ['name', 'display_name', 'real_name'] and returns null immediately when a field produces more than one match. If two users share the same name but exactly one has a matching display_name, the loop warns "ambiguous" and bails before checking display_name. A user who types <@jane> meaning the display name gets an unresolvable warning even though they are unique by display name. Fix: log the ambiguity and continue to the next field instead of returning null.

---

Tests

4. updateMessage() mention resolution is not tested — BotSlackSenderTest.groovy

The existing updateMessage test uses a payload with no <@...> pattern, so hasResolvableMentions returns false and the resolver is never exercised. A regression removing the mentionResolver.resolveInJson() call in updateMessage would not be caught.

5. Webhook warning test verifies dispatch, not behavior — SlackClientTest.groovy line 616

1 * sender.warnIfUnresolvedMentions(...) asserts the method is entered (Spock Spy), but the Spy intercepts the call — it does not verify that log.warn actually fires. If the loggedWarnings.add() guard or the log.warn call inside were broken, this test would still pass.

---

Cleanup

6. Redundant find() + reset() in replaceMentions — SlackMentionResolver.groovy line 124

The method calls matcher.find() once to short-circuit, then matcher.reset() and loops again. A while (matcher.find()) { ... } loop handles the empty case identically without the extra scan.

[adamrtalbot]

Thanks for the feedback — agreed this had grown more complex than a short-lived Nextflow run needs.

What changed in the latest push:

• No cache layer — one lazy users.list pagination the first time a mention is resolved; the list lives on the resolver instance for the pipeline run only (no synchronized cache, no usergroups fetch).
• Users only — dropped subteam / usergroups.list resolution to keep scope tight. Happy to follow up separately if subteam mentions are still wanted.
• Same HTTP pattern as BotSlackSender — direct users.list GET with the bot token; there is no Slack SDK dependency in this repo today.
• Simpler ID detection — already-resolved mentions are skipped when the inner text matches Slack’s uppercase ID format (U/W + 4+ alphanumerics, optional |label). Display names like <@Ulysses> still resolve; lowercase/mixed-case names are not mistaken for IDs.
• Copyright — header is 2025-2026 to match the rest of the plugin.
• Tests/docs trimmed — subteam cases removed; core user mention + JSON payload coverage kept.

Line counts: SlackMentionResolver.groovy 293 → 216; SlackMentionResolverTest.groovy 214 → 175.

./gradlew test — 151 tests, all passing.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> patterns into Slack user IDs before messages are sent. Documentation, examples, and tests are all present and accurate. The feature degrades gracefully on webhooks and caches the users list for the pipeline run.

Two correctness issues to address before merge:

---

Bugs

src/main/groovy/nextflow/slack/SlackMentionResolver.groovy:36 — SLACK_USER_ID regex false-positives on uppercase display names

private static final java.util.regex.Pattern SLACK_USER_ID = ~/^[UW][A-Z0-9]{4,}(\|.+)?$/

Any all-uppercase display name of 5+ chars starting with U or W — e.g. <@UPLOADS>, <@WEBMASTER> — passes isSlackUserId() and is silently treated as an already-resolved ID, leaving the mention unresolved with no warning. Real Slack user IDs have a minimum length of 9 chars; tightening to {8,} would substantially narrow the false-positive window (e.g. ~/^[UW][A-Z0-9]{8,}(\|.+)?$/).

---

src/main/groovy/nextflow/slack/SlackMentionResolver.groovy:39 — users field is not thread-safe

private List<Map> users

loadUsers() does a plain null-check with no volatile or synchronized. If sendMessage and updateMessage are ever called concurrently (e.g. a progress-timer thread and an onComplete callback), both threads can simultaneously observe users == null, each call fetchUsers(), and the JVM memory model gives no guarantee the written value is published to other CPUs. Mark users as volatile and use double-checked locking, or make loadUsers() synchronized.

---

Minor

SlackMentionResolver.groovy:79 — redundant double-scan in resolveInText

The if (!matcher.find()) return guard followed by matcher.reset() is not a logic error (the first match is correctly reprocessed after reset), but it wastes a full regex pass on every string field. The reset() + loop is enough on its own.

WebhookSlackSender.groovy:38 — loggedWarnings Set stores a constant string

private final Set<String> loggedWarnings = Collections.synchronizedSet(new HashSet<String>())

The warning text is always the same compile-time constant, so this Set can only ever hold one entry. An AtomicBoolean would be clearer and cheaper.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> mentions into Slack user IDs before messages are sent, integrates it into BotSlackSender, and adds a warning for webhook users who can't resolve mentions. Docs and unit tests are included. A few issues to address before merging:

---

Correctness

SlackMentionResolver.groovy line ~172 — Partial user list silently cached on mid-pagination failure

If slackGet() fails on any page after the first (network error, 429, 500), fetchUsers() breaks out of the loop with a partial result — but the warning guard if (result.isEmpty()) only fires when nothing was fetched. The partial list is then permanently stored in users and reused for the object's lifetime. Users on un-fetched pages are silently unresolvable with no log message.

Fix: warn whenever slackGet() returns null mid-pagination (i.e., when cursor != null at break), and don't store the partial result (set users = null or return []).

---

SlackMentionResolver.groovy line ~115 — Ambiguity on an intermediate field blocks fallthrough to more-specific fields

In mentionFor(), when display_name is ambiguous (multiple users share it), the function immediately warns and returns <@query> — it never tries real_name, which might uniquely identify the user.

Example: two users share display_name: 'Alex' but have distinct real names 'Alex Smith' and 'Alex Jones'. <@Alex Smith> fails on the display_name pass and never reaches real_name.

Fix: replace return "<@${query}>" inside the matches.size() > 1 block with continue, and move the warn+return to after the loop exhausts all fields with no unique match.

---

Testing

BotSlackSenderTest.groovy — No integration test for the new mention-resolution wiring

BotSlackSender.sendMessage() (line 80) and updateMessage() (line 424) both call mentionResolver.resolveInJson(), but no test in BotSlackSenderTest stubs the resolver, passes a display-name mention through either method, and asserts the resolved payload flows to postToSlack/postUpdate. Under the existing tests, the live network call silently fails and the mention is left unchanged, so the wiring is never actually exercised.

A test using the anonymous-class override pattern (already used in SlackMentionResolverTest) to return stub users would cover both paths.

---

SlackClientTest.groovy line ~55 — Webhook warning test only checks call-site wiring

The test asserts 1 * sender.warnIfUnresolvedMentions(...) via a Spy. This confirms the method is called, but not that the warning was logged or that the dedup-set suppresses duplicates. If the log.warn line were accidentally removed, or if loggedWarnings.add() always returned false, the test still passes.

---

Minor / Plausible

SlackMentionResolver.groovy line 36 — SLACK_USER_ID regex can silently skip resolution of all-uppercase display names

~/^[UW][A-Z0-9]{4,}(\|.+)?$/ matches any 5+ character all-uppercase string starting with U or W (e.g. URGENT, WSUPPORT). Such display names are uncommon but valid; they'd be silently treated as already-resolved IDs with no warning. Adding a length lower-bound closer to real Slack ID lengths (9 chars) would narrow the false-positive window.

SlackMentionResolver.groovy line 158 — Unsynchronized users field

loadUsers() does an unsynchronized if (users == null) check on a non-volatile field. If Nextflow delivers concurrent task-event callbacks on separate threads (which onProcessSubmit/onProcessComplete can do), multiple threads can race into fetchUsers() simultaneously, causing duplicate users.list HTTP calls that may hit Slack's rate limit. Adding synchronized(this) around the null-check-and-assign or making users volatile would fix this.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> → <@U123> via users.list, with caching, pagination, ambiguity warnings, and a webhook-path warning. Documentation and test coverage are solid overall. Four issues worth addressing before merge:

---

Bug — SLACK_USER_ID regex false-positives on all-uppercase display names

File: src/main/groovy/nextflow/slack/SlackMentionResolver.groovy

private static final java.util.regex.Pattern SLACK_USER_ID = ~/^[UW][A-Z0-9]{4,}(\|.+)?$/

Any all-uppercase display name starting with U or W (e.g. UBUNTU, WINDOWS, WORKFLOW) matches this pattern and is treated as an already-resolved Slack ID — no lookup is attempted, no warning is emitted. The existing test uses "Ulysses" (mixed case), which fails the pattern, so this gap isn't caught. Real Slack user IDs also contain digits; tightening the pattern (e.g. requiring at least one digit, or a minimum length of ~8) would reduce false positives.

---

Bug — Partial pagination failure floods log warnings

File: src/main/groovy/nextflow/slack/SlackMentionResolver.groovy

if (!response) {
    if (result.isEmpty()) usersListUnavailable = true  // only set on first-page failure
    break
}

If page 1 succeeds but page 2+ fails, result is non-empty so usersListUnavailable stays false. Every subsequent message with an unresolved mention (anyone in the missing pages) emits a full "no matching Slack user found" warning with no suppression. The usersListUnavailable flag should also be set (or a separate partialUsersLoaded flag used) when mid-pagination fails.

---

Docs — PR description claims subteam resolution but it's not implemented

The PR summary says:

Add SlackMentionResolver to translate <@DisplayName> and <!subteam^TeamName> into Slack user/subteam IDs

The MENTION regex is <@([^>]+)> — it only matches <@...>. The <!subteam^...> syntax uses a ! prefix and is never matched. There is no subteam handling anywhere in the code. The PR description should drop the subteam claim, or issue #46 scope should be updated.

---

Minor — Block-kit Map messages bypass early users:read scope validation

File: src/main/groovy/nextflow/slack/BotSlackSender.groovy

if (config.onStart?.message instanceof String) texts << ...

If a user configures a block-kit Map message containing a display-name mention, collectMentionTexts silently skips it (the instanceof String guard fails). validateMentionConfig never calls verifyUsersReadAccess, so the "add users:read scope" warning is never surfaced at startup — the user only discovers the issue when the first message fails to resolve. The guide's new "Mentioning Users" section shows only string syntax; a note that block-kit Map messages also support mention resolution (and still benefit from users:read) would help.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> syntax into Slack user IDs before messages are sent, with caching, ambiguity warnings, and a startup scope check. The structure is solid and the test coverage is broad. A few correctness issues and doc gaps worth addressing:

---

Bugs

1. Mid-pagination failure silently truncates the user list
SlackMentionResolver.groovy:204

In fetchUsers(), usersListUnavailable is only set when the first page fails (result.isEmpty()). If page 1 succeeds but page 2+ fails, the loop breaks with a partial list, usersListUnavailable stays false, and the truncated list is cached for the whole run. Any user past the first page silently fails to resolve with a misleading "no matching Slack user found" warning — no indication the user list is incomplete.

// Fix: set usersListUnavailable on any pagination failure
if (!response) {
    if (result.isEmpty()) {
        usersListUnavailable = true
    } else {
        log.warn "Slack plugin: users.list pagination failed after ${result.size()} users — @mention resolution may be incomplete"
    }
    break
}

---

2. collectMentionTexts skips Map-format messages — the documented example is bypassed
BotSlackSender.groovy:86

collectMentionTexts only handles instanceof String messages. But the primary documented example uses a Map:

message = [text: ':wave: Hi <@Jane>!']

This silently bypasses validateMentionConfig, so verifyUsersReadAccess() is never called at startup for users following the docs. The mention is still resolved correctly at send time via resolveInJson, but the user misses the early "missing users:read scope" warning.

---

Documentation gaps

3. PR description claims subteam support (<!subteam^TeamName>) — it is not implemented

The PR summary says:

Add SlackMentionResolver to translate <@DisplayName> and <!subteam^TeamName> into Slack user/subteam IDs

But the MENTION regex (<@([^>]+)>) only matches <@...> patterns. <!subteam^Engineering> is not matched, so it would be sent verbatim. Either the feature was cut or the description/docs need updating to reflect the actual scope.

---

4. Example config uses a real contributor username
example/nextflow.config:28

message = 'Hi <@adamtalbot>'

Users copying this example will get a confusing "no matching Slack user found" warning in their own workspace. Use a generic placeholder like <@YourName> instead.

---

Minor

5. SLACK_USER_ID regex minimum length is too loose
SlackMentionResolver.groovy:36

^[UW][A-Z0-9]{4,} requires only 4 chars after the prefix (5 total). Real Slack user IDs are 9–11 characters. A short all-uppercase display name like UTEST would be misclassified as an already-resolved ID and silently skipped. Tightening to {8,} (9 total) would match real IDs while still catching the normal display-name case.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> mentions into Slack user IDs before bot messages are sent, with caching, pagination, and graceful degradation for webhooks. Documentation and example are included, and unit tests cover the resolver logic well. Four issues to address before merge:

---

Bugs

SlackMentionResolver.groovy:34 — Subteam mentions claimed but not implemented
The PR description and summary say <!subteam^TeamName> is supported, but the MENTION regex (~/<@([^>]+)>/) only matches <@...> — there is no usergroups.list call anywhere. Either remove the subteam claim from the PR description, or implement it. Shipping with a false description is worse than a missing feature.

BotSlackSender.groovy — completeUpload bypasses mention resolution
sendMessage and updateMessage call mentionResolver.resolveInJson, but the completeUpload path assigns initial_comment directly without going through the resolver. Display-name mentions in file upload comments are silently sent as literal <@name> strings.

SlackMentionResolver.groovy:216 — Mid-pagination failure silently returns a partial user list

if (!response) {
    if (result.isEmpty()) {   // <-- only sets unavailable if NO users were fetched
        usersListUnavailable = true
        users = result
    }
    break
}

If page 1 succeeds (200+ users) and page 2 fails, the partial list is cached with usersListUnavailable = false and no warning is emitted. Users on subsequent pages silently fail to resolve for the rest of the pipeline run. The if (result.isEmpty()) guard should be removed so that any mid-pagination failure marks the resolver as unreliable.

---

UX

example/nextflow.config:28 — Hardcoded personal Slack handle
message = 'Hi <@adamtalbot>' encodes a real person's display name into a publicly shipped example. Anyone copying this config will either fail silently (wrong workspace) or unexpectedly ping the developer. Replace with a neutral placeholder like <@your.name>.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> mentions into Slack user IDs before messages are sent, with caching, ambiguity detection, and webhook warnings. The documentation and test coverage are solid overall. Three issues worth addressing before merge:

---

1. PR description promises subteam support that isn't implemented

SlackMentionResolver.groovy:34

The PR summary says: "translate <@DisplayName> and <!subteam^TeamName> into Slack user/subteam IDs", but the only pattern defined is:

private static final java.util.regex.Pattern MENTION = ~/<@([^>]+)>/

There is no <!subteam^...> regex, no usergroups.list API call, and no subteam tests anywhere. Either remove the claim from the description or implement it. As written, subteam mentions pass through silently with no warning.

---

2. Token trimming inconsistency between SlackMentionResolver and BotSlackSender

BotSlackSender.groovy:67 / SlackMentionResolver.groovy:47

SlackMentionResolver trims the token in its constructor:

this.botToken = botToken?.trim()

But BotSlackSender stores it raw:

this.botToken = botToken  // no trim

If a token is configured with stray whitespace (e.g. from env var interpolation), SlackMentionResolver resolves mentions correctly while every chat.postMessage / chat.update / files.getUploadURLExternal call in BotSlackSender sends a malformed Authorization header and fails with auth errors. Either trim in SlackConfig before it's passed downstream, or trim in BotSlackSender's constructor.

---

3. Mid-pagination failure silently caches a partial user list

SlackMentionResolver.groovy:192–198

When users.list succeeds on page 1 but fails on page 2+, the guard if (result.isEmpty()) is false, so usersListUnavailable stays false, no warning is emitted, and fetchUsers() returns the truncated list. All users on subsequent pages become permanently unresolvable for the lifetime of the resolver with no diagnostic. A workspace with >200 users is the trigger.

Suggested fix: emit a warning and set usersListUnavailable = true on any page failure, regardless of position:

if (!response) {
    usersListUnavailable = true
    users = result.isEmpty() ? result : result  // always set
    if (!result.isEmpty()) {
        log.warn "Slack plugin: users.list pagination failed mid-page — @mentions may not resolve for all workspace members"
    }
    break
}

---

Minor: redundant find() + reset() in resolveInText

SlackMentionResolver.groovy:83–95

The probe find() at line 84 is discarded and immediately followed by reset(). The appendReplacement/appendTail loop handles the no-match case correctly on its own — the probe adds no value but costs an extra scan per string.

[claude]

Code Review

This PR adds SlackMentionResolver to translate <@DisplayName> patterns into Slack user IDs before messages are sent. The bot sender wires in resolution transparently; the webhook sender warns users they need a bot token. Documentation, examples, and a dedicated test class are all present and accurate.

---

Findings (most severe first)

1. SLACK_USER_ID regex silently skips all-caps display names starting with U/W
src/main/groovy/nextflow/slack/SlackMentionResolver.groovy:36

Pattern ^[UW][A-Z0-9]{4,} matches display names like <@WANDA>, <@URGENT>, <@WASTE> — they pass isSlackUserId() and are never looked up. The comment on line 35 acknowledges that display names may contain lowercase, but the guard only rules out lowercase names; 5+-char all-caps names starting with U/W are misidentified as Slack IDs.

Real Slack IDs are always alphanumeric and at least 9 chars (e.g. U12345678). Tightening the minimum length to 8 would eliminate most false-positives: ~/^[UW][A-Z0-9]{8,}(\|.+)?$/

---

2. Mid-pagination failure silently caches partial user list
src/main/groovy/nextflow/slack/SlackMentionResolver.groovy:196

When page 1 of users.list succeeds but page 2 fails, result is non-empty so the if (result.isEmpty()) guard does not fire: usersListUnavailable stays false, no warning is emitted, and loadUsers() caches the partial list permanently. All users beyond the first page silently fail to resolve for the lifetime of the pipeline run.

Should log a warning on partial failure regardless of result.isEmpty().

---

3. Per-mention "not found" warning has no deduplication
src/main/groovy/nextflow/slack/SlackMentionResolver.groovy:133

The ambiguous and "no matching Slack user found" log paths in mentionFor() fire once per call with no deduplication guard. A workflow sending 100 progress messages containing <@Unknown> produces 100 identical WARN lines. logUsersListError has a one-shot usersListErrorLogged flag; this path needs the same treatment.

---

4. Ambiguity on name field prevents fallthrough to display_name
src/main/groovy/nextflow/slack/SlackMentionResolver.groovy:131

When two users share the same name value, the code logs "Ambiguous" and returns without checking display_name — even if display_name would uniquely identify one user. The PR description says matching is priority-based (username -> display name -> real name), but the bail-out on ambiguity breaks that promise. If the intent is truly priority-based, ambiguity on an earlier field should fall through to the next.

---

Minor

• JSON round-trip format change (SlackMentionResolver.groovy:73): JsonBuilder.toString() produces compact JSON, while SlackMessageBuilder emits pretty-printed. Payloads with mentions come out compact; payloads without stay pretty. No functional impact but inconsistent debug output.
• Test coverage gap: SlackClientTest verifies that warnIfUnresolvedMentions is called but does not assert the warning fires or fires only once. Worth a direct test case once finding docs: Demonstrate how to reply in thread #3 is fixed.